Re: [Libvir] PATCH 6/20: split up qemud_server struct

On Fri, Jun 22, 2007 at 02:48:38AM +0100, Daniel P. Berrange wrote: > This is the 2nd biggest patch of the series. It is a major refactoring of > the data structures, to split qemud_server into a smaller qemud_server > and a new qemud_driver. > > conf.c | 251 ++++++++++---------- > conf.h | 294 +++++++++++++++++++++++- > dispatch.c | 295 ++++++++++-------------- > driver.c | 733 ++++++++++++++++++++++++++++++++++--------------------------- > driver.h | 98 +++----- > internal.h | 236 ------------------- > qemud.c | 50 ---- > 7 files changed, 992 insertions(+), 965 deletions(-) [...] > diff -r 4684eb84957d qemud/conf.h > --- a/qemud/conf.h Thu Jun 21 16:14:19 2007 -0400 > +++ b/qemud/conf.h Thu Jun 21 16:14:44 2007 -0400 > @@ -24,15 +24,277 @@ > #ifndef __QEMUD_CONF_H > #define __QEMUD_CONF_H [...] > + > +static inline int > +qemudIsActiveVM(struct qemud_vm *vm) > +{ > + return vm->id != -1; > +} > + > +static inline int > +qemudIsActiveNetwork(struct qemud_network *network) > +{ > + return network->active; > +} I'm not too fond of this, this assumes a compiler accepting the inlining and or having code being defined in the config file. I way prefer a simple macro, that's more portable.

> diff -r 4684eb84957d qemud/dispatch.c > --- a/qemud/dispatch.c Thu Jun 21 16:14:19 2007 -0400 > +++ b/qemud/dispatch.c Thu Jun 21 16:14:44 2007 -0400 [...] > +extern struct qemud_driver *qemu_driver; hum, is that a declaration or ar reference ? I'm alway a bit suspicious of such construct, if shared it goes in the .h as extern, and without extern in one .c , if not shared it should really be static.

[...] > diff -r 4684eb84957d qemud/driver.c > --- a/qemud/driver.c Thu Jun 21 16:14:19 2007 -0400 > +++ b/qemud/driver.c Thu Jun 21 16:26:12 2007 -0400 > @@ -23,6 +23,8 @@ > > #include <config.h> > > +#define _GNU_SOURCE /* for asprintf */ > + please no, I understand it's painful and error prone to do the calculation of the target string lenght, but that's really not portable.

[...] > + if (asprintf (&base, "%s/.libvirt/qemu", pw->pw_dir) == -1) { > + qemudLog (QEMUD_ERR, "out of memory in asprintf"); > + goto out_of_memory; > + } > + } plus it's for a path names, use a MAX_PATH buffer, snprintf in it and the strdup() it. It won't be that much longuer and avoid the issue

here too we are doing weird stuff to build paths, the erro code aren't checked a single snprintf to a MAX_PATH buffer followed by an strdup() would be cleaner IMHO. I understand it's not something added by the patch here, but probably woth fixing since similar to previous. [...] > + if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/qemu/log", pw->pw_dir) >= PATH_MAX) > + goto snprintf_error; > + So server->logDir is a PATH_MAX array embbeded in the structure, I'm sure we don't need this and can allocate the string instead, no ?

> if (asprintf (&base, "%s/.libvirt/qemu", pw->pw_dir) == -1) { > qemudLog (QEMUD_ERR, "out of memory in asprintf"); > return -1; > } > } one more here. > @@ -711,7 +691,6 @@ static struct qemud_server *qemudInitial > } > > /* We don't have a dom-0, so start from 1 */ > - server->nextvmid = 1; > server->sigread = sigread; shouldn't the comment be removed too then ?