2012/2/1 Jean-Baptiste Rouault <jean-baptiste.rouault@xxxxxxxxxxx>:
> This patch adds an internal function vmwareGetVMStatus to
> get the real state of the domain. This function is used in
> various places in the driver, in particular to detect when
> the domain has been shut down by the user with the "halt"
> command.
> @@ -181,6 +182,50 @@ vmwareGetVersion(virConnectPtr conn, unsigned long *version)
> }
>
> static int
> +vmwareGetVMStatus(struct vmware_driver *driver,
> + virDomainObjPtr vm,
> + int *status,
> + int *reason)
> +{
> + virCommandPtr cmd;
> + char *outbuf;
> + char *vmxAbsolutePath;
vmxAbsolutePath needs to be initialized to NULL and outbuf should be
initialized to NULL too.
> + int state;
> + int ret = -1;
> +
> + cmd = virCommandNewArgList(VMRUN, "-T", vmw_types[driver->type],
> + "list", NULL);
> + virCommandSetOutputBuffer(cmd, &outbuf);
> + if (virCommandRun(cmd, NULL) < 0)
> + goto cleanup;
Because this goto cleanup can skip the initialization of
vmxAbsolutePath and the VIR_FREE call later on will free a random
pointer.
> + state = virDomainObjGetState(vm, reason);
> +
> + if (virFileResolveAllLinks(((vmwareDomainPtr) vm->privateData)->vmxPath,
> + &vmxAbsolutePath) == -1)
Check for < 0 instead of == -1.
> + goto cleanup;
> +
> + if (strstr(outbuf, vmxAbsolutePath)) {
This isn't robust, simple substring match doesn't work properly here.
For example, when vmxAbsolutePath is /random/path/file.vmx and you
have two VMs, /random/path/file.vmx and
/something/random/path/file.vmx. Now just the second one is currently
active, vmrun outputs just /something/random/path/file.vmx and you get
a match for /random/path/file.vmx from strstr making you think this
one is active but it isn't.
I think this needs more complex parsing to get it robust.
> + /* If the vmx path is in the output, the domain is running or
> + * is paused but we have no way to detect if it is paused or not. */
> + if (state == VIR_DOMAIN_PAUSED)
> + *status = state;
> + else
> + *status = VIR_DOMAIN_RUNNING;
> + } else {
> + *status = VIR_DOMAIN_SHUTOFF;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + virCommandFree(cmd);
> + VIR_FREE(outbuf);
> + VIR_FREE(vmxAbsolutePath);
> + return ret;
> +}
> +
> +static int
> vmwareStopVM(struct vmware_driver *driver,
> virDomainObjPtr vm,
> virDomainShutoffReason reason)
> @@ -331,7 +371,10 @@ vmwareDomainShutdownFlags(virDomainPtr dom,
> goto cleanup;
> }
>
> - if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
> + if (vmwareGetVMStatus(driver, vm, &status, NULL) == -1)
> + goto cleanup;
Prefer < 0 over == -1, as this is the common style in libvirt.
You changed all lifecycle functions not to rely on possible stale
information, but you missed to update the cached state information in
the virDomainObj list resulting in virsh list giving wrong output and
possibly listing VMs as active that aren't active anymore.
Maybe you should replace vmwareGetVMStatus with vmwareUpdateVMStatus
that updates the cached information in a virDomainObj and call it in
all places that read information from a virDomainObj.
A more general question: is this driver supposed to work properly when
someone uses vmrun to alter a VMs behind libvirt's back? If that's the
case then maybe this driver should not cache anything at all and work
like the vSphere, Hyper-V or VirtualBox driver. They always query the
hypervisor for information and cache nothing.
I'm not sure what's the best approach here.
--
Matthias Bolte
http://photron.blogspot.com
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list