Re: [libvirt] [PATCH] vmware: detect when a domain was shut down from the inside
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vmware: detect when a domain was shut down from the inside
2012/2/1 Jean-Baptiste Rouault :
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vmware: detect when a domain was shut down from the inside
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 runnin
[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 (virDomainObjIs
