On Fri, Aug 29, 2008 at 05:14:12PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list