"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;
And sometimes it's worthwhile to avoid the duplication of
assignment+LHS. So, this seems slightly more readable/maintainable,
since dom->state appears only once, and the two enum values are even
closer to each other:
dom->state = (STREQ(status, "stopped")
? VIR_DOMAIN_SHUTOFF
: 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
> + char *ret = strdup(vm->def->os.type);
> +
> + if (!ret)
> + openvzError(dom->conn, VIR_ERR_NO_MEMORY, NULL);
> +
> + return ret;
> }
...
> 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.
> if (!vm) {
> openvzError(dom->conn, VIR_ERR_INVALID_DOMAIN,
> - _("no domain with matching id"));
> - return -1;
> - }
...
> @@ -301,26 +297,23 @@ static int openvzDomainReboot(virDomainP
> static int openvzDomainReboot(virDomainPtr dom,
> unsigned int flags ATTRIBUTE_UNUSED) {
> struct openvz_driver *driver = (struct openvz_driver *)dom->conn->privateData;
> - struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> - const char *prog[] = {VZCTL, "--quiet", "restart", vm->vmdef->name, NULL};
> + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> + const char *prog[] = {VZCTL, "--quiet", "restart", vm->def->name, NULL};
same here
> if (!vm) {
> openvzError(dom->conn, VIR_ERR_INVALID_DOMAIN,
> - _("no domain with matching id"));
> - return -1;
> - }
...
> @@ -570,8 +575,8 @@ openvzDomainCreate(virDomainPtr dom)
> openvzDomainCreate(virDomainPtr dom)
> {
> struct openvz_driver *driver = (struct openvz_driver *)dom->conn->privateData;
> - struct openvz_vm *vm = openvzFindVMByName(driver, dom->name);
> - const char *prog[] = {VZCTL, "--quiet", "start", vm->vmdef->name, NULL };
> + virDomainObjPtr vm = virDomainFindByName(driver->domains, dom->name);
> + const char *prog[] = {VZCTL, "--quiet", "start", vm->def->name, NULL };
>
> if (!vm) {
> openvzError(dom->conn, VIR_ERR_INVALID_DOMAIN,
> @@ -579,7 +584,7 @@ openvzDomainCreate(virDomainPtr dom)
> return -1;
> }
>
> - if (vm->status != VIR_DOMAIN_SHUTOFF) {
> + if (vm->state != VIR_DOMAIN_SHUTOFF) {
> openvzError(dom->conn, VIR_ERR_OPERATION_DENIED,
> _("domain is not in shutoff state"));
> return -1;
> @@ -591,10 +596,9 @@ openvzDomainCreate(virDomainPtr dom)
> return -1;
> }
>
> - vm->vpsid = strtoI(vm->vmdef->name);
> - vm->status = VIR_DOMAIN_RUNNING;
> - ovz_driver.num_inactive --;
> - ovz_driver.num_active ++;
> + vm->pid = strtoI(vm->def->name);
> + vm->def->id = vm->pid;
> + vm->state = VIR_DOMAIN_RUNNING;
>
> return 0;
> }
> @@ -604,15 +608,15 @@ openvzDomainUndefine(virDomainPtr dom)
> {
> virConnectPtr conn= dom->conn;
> struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
> - struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> - const char *prog[] = { VZCTL, "--quiet", "destroy", vm->vmdef->name, NULL };
> + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> + const char *prog[] = { VZCTL, "--quiet", "destroy", vm->def->name, NULL };
Same here: handle vm == NULL
> if (!vm) {
> openvzError(conn, VIR_ERR_INVALID_DOMAIN, _("no domain with matching uuid"));
> return -1;
> }
>
> - if (openvzIsActiveVM(vm)) {
> + if (virDomainIsActive(vm)) {
> openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("cannot delete active domain"));
> return -1;
> }
> @@ -623,7 +627,8 @@ openvzDomainUndefine(virDomainPtr dom)
> return -1;
> }
>
> - openvzRemoveInactiveVM(driver, vm);
> + virDomainRemoveInactive(&driver->domains, vm);
> +
> return 0;
> }
>
> @@ -632,8 +637,8 @@ openvzDomainSetAutostart(virDomainPtr do
> {
> virConnectPtr conn= dom->conn;
> struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
> - struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> - const char *prog[] = { VZCTL, "--quiet", "set", vm->vmdef->name,
> + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> + const char *prog[] = { VZCTL, "--quiet", "set", vm->def->name,
And again.
> "--onboot", autostart ? "yes" : "no",
> "--save", NULL };
>
> @@ -655,7 +660,7 @@ openvzDomainGetAutostart(virDomainPtr do
> {
> virConnectPtr conn= dom->conn;
> struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
> - struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> char value[1024];
>
> if (!vm) {
> @@ -663,7 +668,7 @@ openvzDomainGetAutostart(virDomainPtr do
> return -1;
> }
>
> - if (openvzReadConfigParam(strtoI(vm->vmdef->name), "ONBOOT", value, sizeof(value)) < 0) {
> + if (openvzReadConfigParam(strtoI(vm->def->name), "ONBOOT", value, sizeof(value)) < 0) {
> openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("Could not read container config"));
> return -1;
> }
> @@ -692,32 +697,32 @@ static int openvzDomainSetVcpus(virDomai
> static int openvzDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) {
> virConnectPtr conn= dom->conn;
> struct openvz_driver *driver = (struct openvz_driver *) conn->privateData;
> - struct openvz_vm *vm = openvzFindVMByUUID(driver, dom->uuid);
> + virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> char str_vcpus[32];
> - const char *prog[] = { VZCTL, "--quiet", "set", vm->vmdef->name,
> + const char *prog[] = { VZCTL, "--quiet", "set", vm->def->name,
and again
> @@ -735,51 +740,48 @@ static virDrvOpenStatus openvzOpen(virCo
> virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> int flags ATTRIBUTE_UNUSED)
> {
> - struct openvz_vm *vms;
> -
> + struct openvz_driver *driver;
> /*Just check if the user is root. Nothing really to open for OpenVZ */
> - if (getuid()) { // OpenVZ tools can only be used by r00t
> - 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;
> - }
> + if (getuid()) { // OpenVZ tools can only be used by r00t
If you use geteuid, this will work also when EUID==0, but UID!=0.
> + 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(access("/proc/vz/veinfo", F_OK) == -1 ||
> + access("/proc/user_beancounters", F_OK) == -1) {
> + return VIR_DRV_OPEN_DECLINED;
> + }
> +
> + if (VIR_ALLOC(driver) < 0) {
> + openvzError(conn, VIR_ERR_NO_MEMORY, NULL);
> + return VIR_DRV_OPEN_ERROR;
> + }
> +
> + if (!(driver->caps = openvzCapsInit()))
> + goto cleanup;
At first I thought that taking the above "goto cleanup" was a bug,
since openvzFreeDriver dereferences driver->vms, which looked like
it would be uninitialized in the just-malloc'd buffer. Actually, it's ok,
because VIR_ALLOC performs a calloc-like memset-'\0' of the returned buffer.
> + 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.
> got ++;
> }
...
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list