[libvirt] [PATCH] vmware: detect when a domain was shut down from the inside
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. --- src/vmware/vmware_driver.c | 83 +++++++++++++++++++++++++++++++++++++------- 1 files changed, 70 insertions(+), 13 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 56e9d2d..6f75f86 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -28,6 +28,7 @@ #include "datatypes.h" #include "virfile.h" #include "memory.h" +#include "util.h" #include "uuid.h" #include "command.h" #include "vmx.h" @@ -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; + 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; + + state = virDomainObjGetState(vm, reason); + + if (virFileResolveAllLinks(((vmwareDomainPtr) vm->privateData)->vmxPath, + &vmxAbsolutePath) == -1) + goto cleanup; + + if (strstr(outbuf, vmxAbsolutePath)) { + /* 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) @@ -212,12 +257,6 @@ vmwareStartVM(struct vmware_driver *driver, virDomainObjPtr vm) }; const char *vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath; - if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_SHUTOFF) { - vmwareError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not in shutoff state")); - return -1; - } - vmwareSetSentinal(cmd, vmw_types[driver->type]); vmwareSetSentinal(cmd, vmxPath); if (!((vmwareDomainPtr) vm->privateData)->gui) @@ -317,6 +356,7 @@ vmwareDomainShutdownFlags(virDomainPtr dom, { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int status; int ret = -1; virCheckFlags(0, -1); @@ -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; + + if (status != VIR_DOMAIN_RUNNING) { vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", _("domain is not in running state")); goto cleanup; @@ -467,6 +510,7 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags) VMRUN, "-T", PROGRAM_SENTINAL, "reset", PROGRAM_SENTINAL, "soft", NULL }; + int status; int ret = -1; virCheckFlags(0, -1); @@ -485,8 +529,10 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags) vmwareSetSentinal(cmd, vmw_types[driver->type]); vmwareSetSentinal(cmd, vmxPath); + if (vmwareGetVMStatus(driver, vm, &status, NULL) == -1) + goto cleanup; - if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) { + if (status != VIR_DOMAIN_RUNNING) { vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", _("domain is not in running state")); goto cleanup; @@ -582,6 +628,7 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int status; int ret = -1; virCheckFlags(0, -1); @@ -596,7 +643,10 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, goto cleanup; } - if (virDomainObjIsActive(vm)) { + if (vmwareGetVMStatus(driver, vm, &status, NULL) == -1) + goto cleanup; + + if (status != VIR_DOMAIN_SHUTOFF) { vmwareError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is already running")); goto cleanup; @@ -623,6 +673,7 @@ vmwareDomainUndefineFlags(virDomainPtr dom, { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int status; int ret = -1; virCheckFlags(0, -1); @@ -645,7 +696,10 @@ vmwareDomainUndefineFlags(virDomainPtr dom, goto cleanup; } - if (virDomainObjIsActive(vm)) { + if (vmwareGetVMStatus(driver, vm, &status, NULL) == -1) + goto cleanup; + + if (status == VIR_DOMAIN_RUNNING) { vm->persistent = 0; } else { virDomainRemoveInactive(&driver->domains, vm); @@ -902,6 +956,7 @@ vmwareDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int state; int ret = -1; vmwareDriverLock(driver); @@ -914,7 +969,10 @@ vmwareDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) goto cleanup; } - info->state = virDomainObjGetState(vm, NULL); + if (vmwareGetVMStatus(driver, vm, &state, NULL) == -1) + goto cleanup; + + info->state = state; info->cpuTime = 0; info->maxMem = vm->def->mem.max_balloon; info->memory = vm->def->mem.cur_balloon; @@ -949,8 +1007,7 @@ vmwareDomainGetState(virDomainPtr dom, goto cleanup; } - *state = virDomainObjGetState(vm, reason); - ret = 0; + ret = vmwareGetVMStatus(driver, vm, state, reason); cleanup: if (vm) -- 1.7.8.3
On Wed, Feb 01, 2012 at 02:29:37PM +0100, Jean-Baptiste Rouault wrote:
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. --- src/vmware/vmware_driver.c | 83 +++++++++++++++++++++++++++++++++++++------- 1 files changed, 70 insertions(+), 13 deletions(-)
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 56e9d2d..6f75f86 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -28,6 +28,7 @@ #include "datatypes.h" #include "virfile.h" #include "memory.h" +#include "util.h" #include "uuid.h" #include "command.h" #include "vmx.h" @@ -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; + 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; + + state = virDomainObjGetState(vm, reason); + + if (virFileResolveAllLinks(((vmwareDomainPtr) vm->privateData)->vmxPath, + &vmxAbsolutePath) == -1) + goto cleanup; + + if (strstr(outbuf, vmxAbsolutePath)) { + /* 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) @@ -212,12 +257,6 @@ vmwareStartVM(struct vmware_driver *driver, virDomainObjPtr vm) }; const char *vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath;
- if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_SHUTOFF) { - vmwareError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not in shutoff state")); - return -1; - } - vmwareSetSentinal(cmd, vmw_types[driver->type]); vmwareSetSentinal(cmd, vmxPath); if (!((vmwareDomainPtr) vm->privateData)->gui) @@ -317,6 +356,7 @@ vmwareDomainShutdownFlags(virDomainPtr dom, { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int status; int ret = -1;
virCheckFlags(0, -1); @@ -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; + + if (status != VIR_DOMAIN_RUNNING) { vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", _("domain is not in running state")); goto cleanup; @@ -467,6 +510,7 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags) VMRUN, "-T", PROGRAM_SENTINAL, "reset", PROGRAM_SENTINAL, "soft", NULL }; + int status; int ret = -1;
virCheckFlags(0, -1); @@ -485,8 +529,10 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags) vmwareSetSentinal(cmd, vmw_types[driver->type]); vmwareSetSentinal(cmd, vmxPath);
+ if (vmwareGetVMStatus(driver, vm, &status, NULL) == -1) + goto cleanup;
- if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) { + if (status != VIR_DOMAIN_RUNNING) { vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", _("domain is not in running state")); goto cleanup; @@ -582,6 +628,7 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int status; int ret = -1;
virCheckFlags(0, -1); @@ -596,7 +643,10 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, goto cleanup; }
- if (virDomainObjIsActive(vm)) { + if (vmwareGetVMStatus(driver, vm, &status, NULL) == -1) + goto cleanup; + + if (status != VIR_DOMAIN_SHUTOFF) { vmwareError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is already running")); goto cleanup; @@ -623,6 +673,7 @@ vmwareDomainUndefineFlags(virDomainPtr dom, { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int status; int ret = -1;
virCheckFlags(0, -1); @@ -645,7 +696,10 @@ vmwareDomainUndefineFlags(virDomainPtr dom, goto cleanup; }
- if (virDomainObjIsActive(vm)) { + if (vmwareGetVMStatus(driver, vm, &status, NULL) == -1) + goto cleanup; + + if (status == VIR_DOMAIN_RUNNING) { vm->persistent = 0; } else { virDomainRemoveInactive(&driver->domains, vm); @@ -902,6 +956,7 @@ vmwareDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { struct vmware_driver *driver = dom->conn->privateData; virDomainObjPtr vm; + int state; int ret = -1;
vmwareDriverLock(driver); @@ -914,7 +969,10 @@ vmwareDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) goto cleanup; }
- info->state = virDomainObjGetState(vm, NULL); + if (vmwareGetVMStatus(driver, vm, &state, NULL) == -1) + goto cleanup; + + info->state = state; info->cpuTime = 0; info->maxMem = vm->def->mem.max_balloon; info->memory = vm->def->mem.cur_balloon; @@ -949,8 +1007,7 @@ vmwareDomainGetState(virDomainPtr dom, goto cleanup; }
- *state = virDomainObjGetState(vm, reason); - ret = 0; + ret = vmwareGetVMStatus(driver, vm, state, reason);
cleanup: if (vm)
That looks reasonable to me, there is a small hit due to the extra round trip to the server, but I would rather have the opinion of Mathias before pushing this :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2012/2/1 Jean-Baptiste Rouault <jean-baptiste.rouault@diateam.net>:
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
On Friday 03 February 2012 09:42:29 Matthias Bolte wrote:
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.
Hi, The problem here is that contrary to vSphere, Hyper-V or VirtualBox, I have no way to get a list of defined domains as vmrun only lists running domains. So I think I'll do as you suggested and go for a vmwareUpdateVMStatus function. Regards, Jean-Baptiste -- Jean-Baptiste ROUAULT Ingénieur R&D - diateam : Architectes de l'information Phone : +33 (0)9 53 16 02 70 Fax : +33 (0)2 98 050 051
participants (3)
-
Daniel Veillard -
Jean-Baptiste Rouault -
Matthias Bolte