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

[Daniel Veillard]

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.
  Not a blocker though, but that makes me feel uneasy. Let's use a real
function or a macro, it's not like we need to optimize at the tenth of micro
second in libvirt :-)

> 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

> +
> +    /* Configuration paths are either ~/.libvirt/qemu/... (session) or
> +     * /etc/libvirt/qemu/... (system).
> +     */
> +    if (asprintf (&qemu_driver->configDir, "%s", base) == -1)
> +        goto out_of_memory;
> +
> +    if (asprintf (&qemu_driver->autostartDir, "%s/autostart", base) == -1)
> +        goto out_of_memory;
> +
> +    if (asprintf (&qemu_driver->networkConfigDir, "%s/networks", base) == -1)
> +        goto out_of_memory;
> +
> +    if (asprintf (&qemu_driver->networkAutostartDir, "%s/networks/autostart",
> +                  base) == -1)
> +        goto out_of_memory;

[...]
> @@ -499,25 +596,25 @@ int qemudStartVMDaemon(struct qemud_serv
>      } else
>          vm->def->vncActivePort = vm->def->vncPort;
>  
> -    if ((strlen(server->logDir) + /* path */
> +    if ((strlen(driver->logDir) + /* path */
>           1 + /* Separator */
>           strlen(vm->def->name) + /* basename */
>           4 + /* suffix .log */
>           1 /* NULL */) > PATH_MAX) {
>          qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
>                           "config file path too long: %s/%s.log",
> -                         server->logDir, vm->def->name);
> -        return -1;
> -    }
> -    strcpy(logfile, server->logDir);
> +                         driver->logDir, vm->def->name);
> +        return -1;
> +    }
> +    strcpy(logfile, driver->logDir);
>      strcat(logfile, "/");
>      strcat(logfile, vm->def->name);
>      strcat(logfile, ".log");

  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 ?


  I would say, still apply the patch but we must remember to clean up those
paths allocations. Maybe a single vararg function to build up paths and
return the new string would be a good separate cleanup, that we can probably
do at a later stage.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/