Re: [libvirt] PATCH: Switch openvz driver to domain APIs

"Daniel P. Berrange" <berrange(a)redhat.com&gt; 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;

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

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

> + 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;

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