[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Fri, Jun 22, 2007 at 07:16:19AM -0400, Daniel Veillard wrote:
> 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.

This was in the code already - I just moved it from internal.h to conf.h

> > 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.

It is a reference. It is also an evil short term hack. The entire dispatch.c
file is killed off in patch 20, so can ignore it.

> 
> [...]
> > 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.

Again just copying existing code here.

> [...]
> > +        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
> 
[snip]
 
>   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 ?

I'll re-visit all these config file / log file path strings later, so we don't
mix up plain re-factoring with other changes.

> 
> >              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 ?

Yes - it should be moved to the new place where nextvmid is initialized.


Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]