[Libvir] PATCH 1/2: Support QEMU (+KVM) in libvirt

[Daniel P. Berrange]

On Mon, Jan 08, 2007 at 02:08:21AM +0100, Karel Zak wrote:
> On Fri, Jan 05, 2007 at 09:16:54PM +0000, Daniel P. Berrange wrote:
> 
> > +/* Return the default machine type for a given architecture */
> > +static const char *qemudDefaultMachineForArch(const char *arch) {
> > +    int i;
> > +
> > +    for (i = 0 ; i < (int)(sizeof(archs) / sizeof(struct qemu_arch_info)) ; i++) {
> 
> #define NR_ITEMS(x)     (int)(sizeof(x)/ sizeof(*x))

Good idea.

> > +
> > + error:
> > +    /* XXX free all the stuff in the qemud_vm struct, or leave it upto
> > +       the caller ? */
> > +    if (prop)
> > +        free(prop);
> > +    if (obj)
> > +        xmlXPathFreeObject(obj);
> 
>        if (ctxt)
>            xmlXPathFreeContext(ctxt);

Yes, nice bug.

> > +    sprintf(vcpus, "%d", vm->def.vcpus);
> > +
> > +    if (!(*argv = malloc(sizeof(char *) * (*argc +1))))
> > +        goto no_memory;
> > +    if (!((*argv)[++n] = strdup(vm->def.os.binary)))
> > +        goto no_memory;
> > +    if (!((*argv)[++n] = strdup("-M")))
> > +        goto no_memory;
> 
>  Hmm... you reallocate static strings. I think you don't have to care
>  about argv[] if you create it after fork() in a child process.  It
>  seems odd allocate and dealocate it in a parent if you need't it the
>  parent process.

Currently I create the argv[] in the main daemon process, principally
so I can easily log the command line to stdout (the child has stdout
redirected). I might be able to move the argv[] creation to the child
which would let me avoid strdup'ing the static strings. It might lead
to subtle bug in future if someone didn't pay attention of the contract
of the method.

> > +        if (entry->d_name[0] == '.')
> > +            continue;
> 
>  is ".myconfig" forbidden filename? Otherwise:

Yeah, I was thinking to only allow config files / vm names with the
letters a-Z, 0-9, -, _ and requiring that they start with a letter.
Perhaps that's overly/unneccessarily strict though.

> 
>    if (!strcmp(entry->d_name, ".") || !strcmp(entry->d_name, ".."))
> 
> > +/* Simple grow-on-demand string buffer */
> 
>  Hmm... It seems like duplicate code to virBufferAdd() (xml.c)

Yes, this is one of the bits needing refactoring to be shared across
various places in libvirt.

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