[libvirt] PATCH: Switch openvz driver to domain APIs

[Daniel P. Berrange]

On Fri, Aug 29, 2008 at 05:14:12PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at redhat.com> wrote:
> > Here's an updated patch
> ...
> > diff -r e270be59a80f src/openvz_conf.c
> ...
> > +        if (VIR_ALLOC(dom) < 0 ||
> > +            VIR_ALLOC(dom->def) < 0)
> > +            goto no_memory;
> > +
> > +        if (STRNEQ(status, "stopped"))
> > +            dom->state = VIR_DOMAIN_RUNNING;
> > +        else
> > +            dom->state = VIR_DOMAIN_SHUTOFF;
> 
> I prefer to avoid negatives,
> 
>            if (STREQ(status, "stopped"))
>                dom->state = VIR_DOMAIN_SHUTOFF;
>            else
>                dom->state = VIR_DOMAIN_RUNNING;

I made this made.

> > +
> > +        dom->pid = veid;
> > +        dom->def->id = dom->state == VIR_DOMAIN_SHUTOFF ? -1 : veid;
> > +
> ...
> > diff -r e270be59a80f src/openvz_driver.c
> > --- a/src/openvz_driver.c	Wed Aug 27 17:03:25 2008 +0100
> > +++ b/src/openvz_driver.c	Thu Aug 28 09:50:23 2008 +0100
> ...
> > +static char *openvzGetOSType(virDomainPtr dom)
> > +{
> > +    struct  openvz_driver *driver = (struct openvz_driver *)dom->conn->privateData;
> > +    virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> 
> handle a NULL vm

And this change.

> >  static int openvzDomainShutdown(virDomainPtr dom) {
> >      struct openvz_driver *driver = (struct openvz_driver *)dom->conn->privateData;
> > -    struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> > -    const char *prog[] = {VZCTL, "--quiet", "stop", vm->vmdef->name, NULL};
> > +    virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> > +    const char *prog[] = {VZCTL, "--quiet", "stop", vm->def->name, NULL};
> 
> I know it's nothing new, but the above use of "vm" can dereference NULL.

Every 'virDomainPtr' object is /required/ to have a non-NULL 'def' field.
Only the 'newDef' field is alllowed to be NULL.

> > +        return VIR_DRV_OPEN_DECLINED;
> > +    } else {
> > +        if (uri == NULL || uri->scheme == NULL || uri->path == NULL)
> > +            return VIR_DRV_OPEN_DECLINED;
> > +        if (STRNEQ (uri->scheme, "openvz"))
> > +            return VIR_DRV_OPEN_DECLINED;
> > +        if (STRNEQ (uri->path, "/system"))
> > +            return VIR_DRV_OPEN_DECLINED;
> 
> How about the following equivalent code, instead.  Then, readers
> don't have to visually compare the three return statements in order to
> understand that they're all returning the same value:
> 
>            if (uri == NULL
>                || uri->scheme == NULL
>                || uri->path == NULL
>                || STRNEQ (uri->scheme, "openvz")
>                || STRNEQ (uri->path, "/system"))
>                return VIR_DRV_OPEN_DECLINED;

I made that change to.
 
> > +    if (openvzLoadDomains(driver) < 0)
> > +        goto cleanup;
> >                      _("Could not parse VPS ID %s"), buf);
> >              continue;
> >          }
> > -        sprintf(vpsname, "%d", veid);
> > +        snprintf(vpsname, sizeof(vpsname), "%d", veid);
> >          names[got] = strdup(vpsname);
> 
> Not yours (it's in context after all), but we must not put NULL
> in the "names" array, since virsh.c's cmdList function sorts the
> strings in that list.  Hence we must detect failed strdup here.

I put in a check & cleanup for OOM there.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|