subject:"\[libvirt\] \[PATCH 1\/3\] PHYP\: Separating UUID functions in another file"

Re: [libvirt] [PATCH 1/3] PHYP: Separating UUID functions in another file

2010-11-22 Thread Eduardo Otubo


On 11/19/2010 07:37 PM, Eric Blake wrote:

On 11/19/2010 07:55 AM, Eduardo Otubo wrote:

I am moving all the UUID handling functions to phyp_uuid.[ch] files in
order not to bloat the main files phyp_driver.[ch] too much. Doing this
for two reasons:

 1) Network management in pHyp does not have a UUID.
 2) Need to create another set of functions to manage it.

I also modified some functions to support two types of execution:
DOMAIN and NET, so I can re-use the base common functions.
---
  po/POTFILES.in |1 +
  src/Makefile.am|3 +-
  src/phyp/phyp_driver.c |  464 +-
  src/phyp/phyp_driver.h |   41 +++
  src/phyp/phyp_uuid.c   |  657 
  src/phyp/phyp_uuid.h   |   36 +++
  6 files changed, 742 insertions(+), 460 deletions(-)
  create mode 100644 src/phyp/phyp_uuid.c
  create mode 100644 src/phyp/phyp_uuid.h



[I've rearranged my review a bit; .h before .c]


diff --git a/src/phyp/phyp_uuid.h b/src/phyp/phyp_uuid.h
new file mode 100644
index 000..ddf28f4
--- /dev/null
+++ b/src/phyp/phyp_uuid.h
@@ -0,0 +1,36 @@
+
+/*
+ * Copyright (C) 2010 Red Hat, Inc.
+ * Copyright IBM Corp. 2010
+ *
+ * phyp_uuid.c: set of functions to handle lpar uuid and network uuid
+ *  which does not have uuid itself, it must be artificially
+ *  created.
+ *

...

+
+#includeconfig.h


While there are other counter-examples currently in libvirt.git, the
general rule of thumb tends to be that .c files should include config.h
first before any headers, and therefore .h files should not include it
(because it will already have been included by the .c file including
this .h).



And regarding all your comments, I'll fix and reply in my next patch 
using virInterface API. Thank you very much for all the comments. :)


Regards,

--
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems  Technology Group
Mobile: +55 19 8135 0885
eot...@linux.vnet.ibm.com

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/3] PHYP: Separating UUID functions in another file

2010-11-19 Thread Eduardo Otubo

I am moving all the UUID handling functions to phyp_uuid.[ch] files in
order not to bloat the main files phyp_driver.[ch] too much. Doing this
for two reasons:

1) Network management in pHyp does not have a UUID.
2) Need to create another set of functions to manage it.

I also modified some functions to support two types of execution:
DOMAIN and NET, so I can re-use the base common functions.
---
 po/POTFILES.in |1 +
 src/Makefile.am|3 +-
 src/phyp/phyp_driver.c |  464 +-
 src/phyp/phyp_driver.h |   41 +++
 src/phyp/phyp_uuid.c   |  657 
 src/phyp/phyp_uuid.h   |   36 +++
 6 files changed, 742 insertions(+), 460 deletions(-)
 create mode 100644 src/phyp/phyp_uuid.c
 create mode 100644 src/phyp/phyp_uuid.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 2820ac1..e892d0b 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -50,6 +50,7 @@ src/opennebula/one_driver.c
 src/openvz/openvz_conf.c
 src/openvz/openvz_driver.c
 src/phyp/phyp_driver.c
+src/phyp/phyp_uuid.c
 src/qemu/qemu_bridge_filter.c
 src/qemu/qemu_conf.c
 src/qemu/qemu_driver.c
diff --git a/src/Makefile.am b/src/Makefile.am
index a9a1986..608b913 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -248,7 +248,8 @@ SECURITY_DRIVER_APPARMOR_HELPER_SOURCES =   
\
security/virt-aa-helper.c
 
 PHYP_DRIVER_SOURCES =  \
-   phyp/phyp_driver.c phyp/phyp_driver.h
+   phyp/phyp_driver.c phyp/phyp_driver.h \
+   phyp/phyp_uuid.c phyp/phyp_uuid.h
 
 OPENVZ_DRIVER_SOURCES =\
openvz/openvz_conf.c openvz/openvz_conf.h   \
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 4c723a2..6f3f49d 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -59,8 +59,10 @@
 #include storage_conf.h
 #include nodeinfo.h
 #include files.h
+#include network_conf.h
 
 #include phyp_driver.h
+#include phyp_uuid.h
 
 #define VIR_FROM_THIS VIR_FROM_PHYP
 
@@ -75,7 +77,7 @@
 static unsigned const int HMC = 0;
 static unsigned const int IVM = 127;
 
-static int
+int
 waitsocket(int socket_fd, LIBSSH2_SESSION * session)
 {
 struct timeval timeout;
@@ -307,7 +309,7 @@ phypCapsInit(void)
  *   1 - Not Activated
  *   * - All
  * */
-static int
+int
 phypNumDomainsGeneric(virConnectPtr conn, unsigned int type)
 {
 ConnectionData *connection_data = conn-networkPrivateData;
@@ -371,7 +373,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type)
  * type: 0 - Running
  *   1 - all
  * */
-static int
+int
 phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
unsigned int type)
 {
@@ -432,462 +434,6 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int 
nids,
 }
 
 static int
-phypUUIDTable_WriteFile(virConnectPtr conn)
-{
-phyp_driverPtr phyp_driver = conn-privateData;
-uuid_tablePtr uuid_table = phyp_driver-uuid_table;
-unsigned int i = 0;
-int fd = -1;
-char local_file[] = ./uuid_table;
-
-if ((fd = creat(local_file, 0755)) == -1)
-goto err;
-
-for (i = 0; i  uuid_table-nlpars; i++) {
-if (safewrite(fd, uuid_table-lpars[i]-id,
-  sizeof(uuid_table-lpars[i]-id)) !=
-sizeof(uuid_table-lpars[i]-id)) {
-VIR_ERROR0(_(Unable to write information to local file.));
-goto err;
-}
-
-if (safewrite(fd, uuid_table-lpars[i]-uuid, VIR_UUID_BUFLEN) !=
-VIR_UUID_BUFLEN) {
-VIR_ERROR0(_(Unable to write information to local file.));
-goto err;
-}
-}
-
-if (VIR_CLOSE(fd)  0) {
-virReportSystemError(errno, _(Could not close %s),
- local_file);
-goto err;
-}
-return 0;
-
-  err:
-VIR_FORCE_CLOSE(fd);
-return -1;
-}
-
-static int
-phypUUIDTable_Push(virConnectPtr conn)
-{
-ConnectionData *connection_data = conn-networkPrivateData;
-LIBSSH2_SESSION *session = connection_data-session;
-LIBSSH2_CHANNEL *channel = NULL;
-virBuffer username = VIR_BUFFER_INITIALIZER;
-struct stat local_fileinfo;
-char buffer[1024];
-int rc = 0;
-FILE *fd;
-size_t nread, sent;
-char *ptr;
-char local_file[] = ./uuid_table;
-char *remote_file = NULL;
-
-if (conn-uri-user != NULL) {
-virBufferVSprintf(username, %s, conn-uri-user);
-
-if (virBufferError(username)) {
-virBufferFreeAndReset(username);
-virReportOOMError();
-goto err;
-}
-}
-
-if (virAsprintf
-(remote_file, /home/%s/libvirt_uuid_table,
- virBufferContentAndReset(username))
- 0) {
-virReportOOMError();
-goto err;
-}
-
-if (stat(local_file, local_fileinfo) == -1) {
-VIR_WARN0(Unable to stat local

Re: [libvirt] [PATCH 1/3] PHYP: Separating UUID functions in another file

2010-11-19 Thread Eric Blake

On 11/19/2010 07:55 AM, Eduardo Otubo wrote:
 I am moving all the UUID handling functions to phyp_uuid.[ch] files in
 order not to bloat the main files phyp_driver.[ch] too much. Doing this
 for two reasons:
 
 1) Network management in pHyp does not have a UUID.
 2) Need to create another set of functions to manage it.
 
 I also modified some functions to support two types of execution:
 DOMAIN and NET, so I can re-use the base common functions.
 ---
  po/POTFILES.in |1 +
  src/Makefile.am|3 +-
  src/phyp/phyp_driver.c |  464 +-
  src/phyp/phyp_driver.h |   41 +++
  src/phyp/phyp_uuid.c   |  657 
 
  src/phyp/phyp_uuid.h   |   36 +++
  6 files changed, 742 insertions(+), 460 deletions(-)
  create mode 100644 src/phyp/phyp_uuid.c
  create mode 100644 src/phyp/phyp_uuid.h
 

[I've rearranged my review a bit; .h before .c]

 diff --git a/src/phyp/phyp_uuid.h b/src/phyp/phyp_uuid.h
 new file mode 100644
 index 000..ddf28f4
 --- /dev/null
 +++ b/src/phyp/phyp_uuid.h
 @@ -0,0 +1,36 @@
 +
 +/*
 + * Copyright (C) 2010 Red Hat, Inc.
 + * Copyright IBM Corp. 2010
 + *
 + * phyp_uuid.c: set of functions to handle lpar uuid and network uuid
 + *  which does not have uuid itself, it must be artificially
 + *  created.
 + *
...
 +
 +#include config.h

While there are other counter-examples currently in libvirt.git, the
general rule of thumb tends to be that .c files should include config.h
first before any headers, and therefore .h files should not include it
(because it will already have been included by the .c file including
this .h).

 +
 +int phypUUIDTable_Init(virConnectPtr conn);

Where is virConnectPtr defined?  This header should be self-contained,
rather than relying on the .c file to include pre-requisite headers.

 +
 +void phypUUIDTable_Free(uuid_tablePtr uuid_table);

Where is uuid_tablePtr defined?  Did you mean int?  Or should
phypUUIDTable_Init be returning a uuid_tablePtr instead of an int?

 +
 +int phypUUIDTable_RemLpar(virConnectPtr conn, int id);
 +
 +int phypUUIDTable_AddLpar(virConnectPtr conn, unsigned char *uuid,
int id);

 diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h
 index bc8e003..603d048 100644
 --- a/src/phyp/phyp_driver.h
 +++ b/src/phyp/phyp_driver.h
 @@ -34,6 +34,7 @@
  # define LPAR_EXEC_ERR -1
  # define SSH_CONN_ERR -2 /* error while trying to connect to
remote host */
  # define SSH_CMD_ERR -3  /* error while trying to execute the
remote cmd */
 +# define NETNAME_SIZE 24

Is this adequate?  What's your rationale for this size?


  typedef struct _ConnectionData ConnectionData;
  typedef ConnectionData *ConnectionDataPtr;
 @@ -42,6 +43,28 @@ struct _ConnectionData {
  int sock;
  };

 +
 +/* This is the network struct that relates
 + * the MAC with UUID generated by the API
 + * */
 +typedef struct _net net_t;
 +typedef net_t *netPtr;
 +struct _net {
 +unsigned char uuid[VIR_UUID_BUFLEN];
 +long long mac;

Typically a mac is 6 bytes, not 8.  Is sign extension going to be a problem?

 +char name[NETNAME_SIZE];
 +};
 +
 +/* Struct that holds how many networks we're
 + * handling and a pointer to an array of net structs
 + * */
 +typedef struct _uuid_nettable uuid_nettable_t;
 +typedef uuid_nettable_t *uuid_nettablePtr;
 +struct _uuid_nettable {
 +int nnets;

s/int/size_t/

 +netPtr *nets;
 +};
 +
  /* This is the lpar (domain) struct that relates
   * the ID with UUID generated by the API
   * */
 @@ -68,6 +91,7 @@ typedef struct _phyp_driver phyp_driver_t;
  typedef phyp_driver_t *phyp_driverPtr;
  struct _phyp_driver {
  uuid_tablePtr uuid_table;
 +uuid_nettablePtr uuid_nettable;
  virCapsPtr caps;
  int vios_id;

 @@ -81,4 +105,21 @@ struct _phyp_driver {

  int phypRegister(void);

 +
 +/*
 + * Functions used in the phyp_uuid.c and must be visible outside
phyp_driver.c
 + * */
 +int phypNumDomainsGeneric(virConnectPtr conn, unsigned int type);
 +
 +int phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
 +   unsigned int type);
 +
 +int waitsocket(int socket_fd, LIBSSH2_SESSION * session);

Why is this not in the phyp namespace?

 +
 +int phypNumOfNetworks(virConnectPtr conn);
 +
 +int phypListNetworks(virConnectPtr conn, char **const names, int nnames);
 +
 +int phypListNetworkMAC(virConnectPtr conn, long long *macs, int nnets);
 +
  #endif /* PHYP_DRIVER_H */



 diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
 index 4c723a2..6f3f49d 100644
 --- a/src/phyp/phyp_driver.c
 +++ b/src/phyp/phyp_driver.c
 @@ -59,8 +59,10 @@
  #include storage_conf.h
  #include nodeinfo.h
  #include files.h
 +#include network_conf.h
  
  #include phyp_driver.h
 +#include phyp_uuid.h
  
  #define VIR_FROM_THIS VIR_FROM_PHYP
  
 @@ -75,7 +77,7 @@
  static unsigned const int HMC = 0;
  static unsigned const int IVM = 127;
  
 -static int
 +int

Re: [libvirt] [PATCH 1/3] PHYP: Separating UUID functions in another file

[libvirt] [PATCH 1/3] PHYP: Separating UUID functions in another file

Re: [libvirt] [PATCH 1/3] PHYP: Separating UUID functions in another file

3 matches

Site Navigation

Mail list logo

Footer information