[libvirt] VMware Workstation/Player support
Matthias Bolte
matthias.bolte at googlemail.com
Wed Nov 17 22:32:27 UTC 2010
2010/11/15 Jean-Baptiste Rouault <jean-baptiste.rouault at diateam.net>:
> On Sunday 14 November 2010 23:11:22 Matthias Bolte wrote:
>> 2010/11/12 Jean-Baptiste Rouault <jean-baptiste.rouault at diateam.net>:
>> > On Tuesday 27 July 2010 20:42:01 Matthias Bolte wrote:
>> >> We'll need to move the VMX handling code from src/esx to src/util,
>> >> because drivers are not allowed to depend on each other.
>> >>
>> >> That should be possible, but will require some refactoring, because
>> >> this code is currently closely entangled with the reset of the ESX
>> >> driver in some places and parts of it are probably quite ESX specific.
>> >
>> > Hi,
>> >
>> > As of today we have a "proof of concept" vmware driver with
>> > basic functions implemented (open/close, define/undefine, create,
>> > start/shutdown...)
>> > However it currently depends on the esx driver for the VMX handling code.
>> > Has anyone started to think of the needed refactoring ? If any help is
>> > needed to refactor I guess I could find some time to work on it.
>> >
>> > Jean-Baptiste
>>
>> A while ago I started to refactor some obviously ESX specific stuff
>> out of the VMX code into a callback mechanism.
>>
>> One thing I'm going to change is the product version handling.
>>
>> There should be no major problem left in the VMX code that could stop
>> it from being moved to src/util/.
>>
>> As you say you have some basic stuff working now, maybe it's a good
>> idea that you show you code here for an initial review and let me do
>> the refactoring once I can see how you use the VMX code.
>>
>> Matthias
>
> Here is the patch, I can try to split it if it's easier for the review.
> The major part of this patch was done by an old colleague of mine,
> there is still work and a bit of cleaning to do.
>
> Regards,
>
> Jean-Baptiste
>
> diff --git a/configure.ac b/configure.ac
> index d21d558..90b3181 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -227,6 +227,8 @@ AC_ARG_WITH([uml],
> AC_HELP_STRING([--with-uml], [add UML support @<:@default=check@:>@]),[],[with_uml=check])
> AC_ARG_WITH([openvz],
> AC_HELP_STRING([--with-openvz], [add OpenVZ support @<:@default=yes@:>@]),[],[with_openvz=yes])
> +AC_ARG_WITH([vmware],
> + AC_HELP_STRING([--with-vmware], [add VMware support @<:@default=yes@:>@]),[],[with_wmware=yes])
Typo here, with_wmware=yes should be with_vmware=yes
> AC_ARG_WITH([libssh2],
> AC_HELP_STRING([--with-libssh2=@<:@PFX@:>@], [libssh2 location @<:@default=/usr/local/lib@:>@]),[],[with_libssh2=yes])
> AC_ARG_WITH([phyp],
> @@ -316,6 +318,10 @@ if test "$with_openvz" = "yes"; then
> fi
> AM_CONDITIONAL([WITH_OPENVZ], [test "$with_openvz" = "yes"])
>
> +if test "$with_vmware" = "yes"; then
> + AC_DEFINE_UNQUOTED([WITH_VMWARE], 1, [whether VMware driver is enabled])
> +fi
> +AM_CONDITIONAL([WITH_VMWARE], [test "$with_vmware" = "yes"])
>
> dnl
> dnl check for XDR
> @@ -2274,6 +2280,7 @@ AC_MSG_NOTICE([ Xen: $with_xen])
> AC_MSG_NOTICE([ QEMU: $with_qemu])
> AC_MSG_NOTICE([ UML: $with_uml])
> AC_MSG_NOTICE([ OpenVZ: $with_openvz])
> +AC_MSG_NOTICE([ VMware: $with_vmware])
> AC_MSG_NOTICE([ VBox: $with_vbox])
> AC_MSG_NOTICE([ XenAPI: $with_xenapi])
> AC_MSG_NOTICE([ LXC: $with_lxc])
IMHO "VMware" as name for this driver is too generic, but I don't have
a better alternative, so we'll probably just stick with it.
> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index eaeb477..de05d0f 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -52,6 +52,7 @@ typedef enum {
> VIR_FROM_TEST, /* Error from test driver */
> VIR_FROM_REMOTE, /* Error from remote driver */
> VIR_FROM_OPENVZ, /* Error from OpenVZ driver */
> + VIR_FROM_VMWARE, /* Error from Vmware driver */
s/Vmware/VMware/
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 3c8bf30..6bf490a 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -364,6 +367,12 @@ virInitialize(void)
> # ifdef WITH_OPENVZ
> virDriverLoadModule("openvz");
> # endif
> +# ifdef WITH_VMWARE
> + virDriverLoadModule("vmware");
> +# endif
> +# ifdef WITH_VMWARE
> + virDriverLoadModule("vmware");
> +# endif
One call to virDriverLoadModule for each driver is enough.
> diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c
> new file mode 100644
> index 0000000..4036d9b
> --- /dev/null
> +++ b/src/vmware/vmware_conf.c
> @@ -0,0 +1,447 @@
> +/*---------------------------------------------------------------------------*/
> +/* Copyright 2010, diateam (www.diateam.net)
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +/*---------------------------------------------------------------------------*/
> +
> +#include <config.h>
> +
> +#include <sys/utsname.h>
So this driver is going to be Linux-only. Any intention to make it
work on Windows too?
> +#include "memory.h"
> +#include "nodeinfo.h"
> +#include "uuid.h"
> +#include "virterror_internal.h"
> +
> +#include "vmware_conf.h"
> +
> +#define VMWARE_MAX_ARG 20
> +
> +/* Free all memory associated with a vmware_driver structure */
> +void
> +vmwareFreeDriver(struct vmware_driver *driver)
> +{
> + if (!driver)
> + return;
> +
> + virDomainObjListDeinit(&driver->domains);
> + virCapabilitiesFree(driver->caps);
> + VIR_FREE(driver);
> +}
> +
> +virCapsPtr
> +vmwareCapsInit(void)
> +{
> + struct utsname utsname;
> +
> + virCapsPtr caps;
> +
> + virCapsGuestPtr guest;
> +
> + uname(&utsname);
> +
> + if ((caps = virCapabilitiesNew(utsname.machine, 0, 0)) == NULL)
> + goto no_memory;
> +
> + if (nodeCapsInitNUMA(caps) < 0)
> + goto no_memory;
> +
> + virCapabilitiesSetMacPrefix(caps, (unsigned char[]) {
> + 0x52, 0x54, 0x00});
> +
> + if ((guest = virCapabilitiesAddGuest(caps,
> + "hvm",
> + utsname.machine,
> + sizeof(int) == 4 ? 32 : 8,
I don't think that you can use utsname.machine to determine the
supported architectures, as you can install and run a 32bit VMware
Player/Workstation on a 64bit system and only use 32bit guests. Or am
I wrong here?
I think you need to query this information somehow for the installed
VMware Player/Workstation.
Also the sizeof(int) wordsize detection seems wrong to me. I found
that LXC, OpenVZ and Phyp driver include the same line.
> + NULL, NULL, 0, NULL)) == NULL)
> + goto no_memory;
> +
> + if (virCapabilitiesAddGuestDomain(guest,
> + "vmware",
> + NULL, NULL, 0, NULL) == NULL)
> + goto no_memory;
The driver uses privateData in virDomainObj, but you don't set
caps->privateDataAllocFunc and caps->privateDataFreeFunc. Therefore,
you leak the privateData memory.
See the QEMU and LXC driver how to use this.
> + return caps;
> +
> + no_memory:
> + virCapabilitiesFree(caps);
> + return NULL;
> +}
> +
> +int
> +vmwareLoadDomains(struct vmware_driver *driver)
> +{
> + FILE *fp;
> +
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> + virDomainObjPtr dom = NULL;
> +
> + char vmxPath[100];
> +
> + pvmwareDomain pDomain;
> +
> + char *directoryName = NULL;
> +
> + char *fileName = NULL;
> +
> + char *separator = NULL;
Remove the empty lines between the variables.
> + fp = driver->type ==
> + TYPE_PLAYER ? popen(VMRUN " -T " "player" " list 2>/dev/null",
> + "r") : popen(VMRUN " -T " "ws"
> + " list 2>/dev/null", "r");
> + if (fp == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed"));
> + return -1;
> + }
> +
> + while (!feof(fp)) {
> + if (fscanf(fp, "%s\n", vmxPath) != 1) {
fscanf to an 100 char buffer is asking for a buffer overflow. Using
fgets with a 1k sized buffer might be a better approach here.
> + if (feof(fp))
> + break;
> +
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to parse vmrun list output"));
> + goto cleanup;
> + }
> + if (vmxPath[0] != '/')
> + continue;
> +
> + if (VIR_ALLOC(dom) < 0)
> + goto no_memory;
> +
> + if (virMutexInit(&dom->lock) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot initialize mutex"));
> + VIR_FREE(dom);
> + goto cleanup;
> + }
> + printf("b\n");
> + virDomainObjLock(dom);
> +
> +
> + //* save vmxpath in domain object * //
> + //pDomain = malloc(sizeof(vmwareDomain));
> + //if(pDomain==NULL) {
> + if (VIR_ALLOC(pDomain) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, _("no memory "));
> + goto cleanup;
> + }
> + pDomain->vmxPath = strdup(vmxPath);
Check the result of strdup for OOM (pDomain->vmxPath == NULL).
> + dom->privateData = pDomain;
> +
> +
> + if (VIR_ALLOC(dom->def) < 0)
> + goto no_memory;
> +
> + //vmrun list only reports running vms
> + dom->state = VIR_DOMAIN_RUNNING;
> +
> + dom->refs = 1;
> +
> + //dom->def->id =
> + dom->persistent = 1;
> + /* name */
> + if (vmwareParsePath(vmxPath, &directoryName, &fileName) < 0) {
> + goto cleanup;
> + }
> + separator = strrchr(fileName, '.');
> + if (separator == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("name parsing error"));
> + goto cleanup;
> + }
> + *separator = '\0';
> + if (virAsprintf(&dom->def->name, "%s", fileName) < 0)
> + goto no_memory;
> +
> + if (virUUIDGenerate(dom->def->uuid) < 0)
> + goto no_memory;
> + dom->def->vcpus = 1;
> +
> + virUUIDFormat(dom->def->uuid, uuidstr);
> + if (virHashAddEntry(driver->domains.objs, uuidstr, dom) < 0)
> + goto no_memory;
Don't allocate a virDomainObj on your own. Also don't make up the
domain name from the VMX filename and don't generate a new UUID. All
this information should be taken from the content of the VMX file.
Use the VMX file parser to obtain a virDomainDef from the VMX file and
add a new domain to the driver->domains list using virDomainAssignDef.
> + virDomainObjUnlock(dom);
> + dom = NULL;
> + }
> +
> + fclose(fp);
> + VIR_FREE(directoryName);
> + VIR_FREE(fileName);
> +
> + return 0;
> +
> + no_memory:
> + virReportOOMError();
> +
> + cleanup:
> + fclose(fp);
> + VIR_FREE(directoryName);
> + VIR_FREE(fileName);
> + if (dom)
> + virDomainObjUnref(dom);
> + return -1;
> +}
> +int
> +vmwareExtractVersion(struct vmware_driver *driver)
> +{
> + unsigned long version = 0;
> +
> + FILE *fp = NULL;
> +
> + char str[50];
> +
> + char *tmp;
> +
> + if (driver->type == TYPE_PLAYER) {
> + if ((fp = popen("vmplayer -v 2>/dev/null", "r")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", ("popen failed"));
> + return -1;
> + }
> + if (!feof(fp) && fgets(str, 49, fp)) {
> + if ((tmp = STRSKIP(str, "VMware Player ")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + ("vmplayer -v parsing error"));
> + return -1;
> + }
> + if (virParseVersionString(tmp, &version) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + ("version parsing error"));
> + return -1;
> + }
> + driver->version = version;
> + return 0;
> + }
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + ("vmplayer -v reading error"));
> + } else if (driver->type == TYPE_WORKSTATION) {
> + if ((fp = popen("vmware -v 2>/dev/null", "r")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", ("popen failed"));
> + return -1;
> + }
> + if (!feof(fp) && fgets(str, 49, fp)) {
> + if ((tmp = STRSKIP(str, "VMware Workstation ")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + ("vmware -v parsing error"));
> + return -1;
> + }
> + if (virParseVersionString(tmp, &version) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + ("version parsing error"));
> + return -1;
> + }
> + driver->version = version;
> + return 0;
> + }
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + ("vmware -v reading error"));
> + }
> + return -1;
All error messages in this function miss the _ before the () around the message.
Also fclose(fp) is missing and we are currently deprecating fclose in
libvirt, use the VIR_FCLOSE macro instead.
> +
> +int
> +vmwareVmxPath(virDomainDefPtr vmdef, char **vmxPath)
> +{
> + virDomainDiskDefPtr disk = NULL;
> +
> + char *directoryName = NULL;
> +
> + char *fileName = NULL;
> +
> + int ret = -1;
> +
> + int i = 0;
> +
> + /*
> + * Build VMX URL. Use the source of the first file-based harddisk
> + * to deduce the path for the VMX file. Don't just use the
> + * first disk, because it may be CDROM disk and ISO images are normaly not
> + * located in the virtual machine's directory. This approach
> + * isn't perfect but should work in the majority of cases.
> + */
> + if (vmdef->ndisks < 1) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _
> + ("Domain XML doesn't contain any disks, cannot deduce "
> + "datastore and path for VMX file"));
> + goto cleanup;
> + }
> +
> + for (i = 0; i < vmdef->ndisks; ++i) {
> + if (vmdef->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
> + vmdef->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) {
> + disk = vmdef->disks[i];
> + break;
> + }
> + }
> +
> + if (disk == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _
> + ("Domain XML doesn't contain any file-based harddisks, "
> + "cannot deduce datastore and path for VMX file"));
> + goto cleanup;
> + }
> +
> + if (disk->src == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _
> + ("First file-based harddisk has no source, cannot deduce "
> + "datastore and path for VMX file"));
> + goto cleanup;
> + }
> +
> + if (vmwareParsePath(disk->src, &directoryName, &fileName) < 0) {
> + goto cleanup;
> + }
> +
> + if (!virFileHasSuffix(fileName, ".vmdk")) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _
> + ("Expecting source '%s' of first file-based harddisk to "
> + "be a VMDK image"), disk->src);
> + goto cleanup;
> + }
> +
> + if (vmwareConstructVmxPath(directoryName, vmdef->name, vmxPath) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(directoryName);
> + VIR_FREE(fileName);
> + return ret;
> +}
In this function _() is split on multiple lines in an unusual way
_\n(). I suggest to stick to the usual way of _().
> diff --git a/src/vmware/vmware_conf.h b/src/vmware/vmware_conf.h
> new file mode 100644
> index 0000000..386ebf2
> --- /dev/null
> +++ b/src/vmware/vmware_conf.h
> +
> +typedef struct _vmwareDomain {
> + char *vmxPath;
> + int z;
z seems unused, remove it?
> + bool gui;
> +
> +} vmwareDomain, *pvmwareDomain;
I suggest renmaing pvmwareDomain to vmwareDomainPtr to stick to the
libvirt naming patterns.
> diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
> new file mode 100644
> index 0000000..7a40ae0
> --- /dev/null
> +++ b/src/vmware/vmware_driver.c
> +
> +static virDrvOpenStatus
> +vmwareOpen(virConnectPtr conn,
> + virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> + int flags ATTRIBUTE_UNUSED)
> +{
> + struct vmware_driver *driver;
> +
> + if (conn->uri == NULL) {
> + /* @TODO accept */
> + return VIR_DRV_OPEN_DECLINED;
> + } else {
> + if (conn->uri->scheme == NULL ||
> + (STRNEQ(conn->uri->scheme, "vmwareplayer") &&
> + STRNEQ(conn->uri->scheme, "vmwarews")))
> + return VIR_DRV_OPEN_DECLINED;
> +
> + /* If server name is given, its for remote driver */
> + if (conn->uri->server != NULL)
> + return VIR_DRV_OPEN_DECLINED;
> +
> + /* If path isn't /system, then they typoed, so tell them correct path */
> + if (conn->uri->path == NULL || STRNEQ(conn->uri->path, "/system")) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + ("unexpected VMware URI path '%s', try vmwareplayer:///system or vmwarews:///system"),
> + conn->uri->path);
> + return VIR_DRV_OPEN_ERROR;
Isn't this more in the sense of /session than /system? As /system
means that there is one system-wide management instance, but VMware
Player/WS operate at a per user level, don't they?
> +static int
> +vmwareDomainCreate(virDomainPtr dom)
> +{
> +
> + vmwareSetSentinal(cmd, vmw_types[driver->type]);
> + vmwareSetSentinal(cmd, ((pvmwareDomain) vm->privateData)->vmxPath);
> + if (((pvmwareDomain) vm->privateData)->gui == false)
Don't compare to false, use the ! operator instead.
> + vmwareSetSentinal(cmd, NOGUI);
> + else
> + vmwareSetSentinal(cmd, NULL);
> +
> + if (virRun(cmd, NULL) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VMRUN);
> + goto cleanup;
> + }
> +
> + virStrToLong_i(vm->def->name, NULL, 10, &vm->pid);
> + vm->def->id = vm->pid;
Parsing a non-existing number from the domain name will not result in
a proper ID.
libvirt requires that the domain ID is unique and >= 0 for non-stopped
domains and -1 for stopped domains.
> + vm->state = VIR_DOMAIN_RUNNING;
> + ret = 0;
> +
> + cleanup:
> + if (vm)
> + virDomainObjUnlock(vm);
> + return ret;
> +}
> +
> +static char *
> +formatVMXFileName(const char *datastorePath, void *opaque)
> +{
> + (void)opaque;
Use ATTRIBUTE_UNUSED instead of this.
> + char *path = NULL;
> + int len = strlen(datastorePath);
> +
> + if (VIR_ALLOC_N(path, len) < 0) {
> + virReportOOMError();
> + return NULL;
> + }
> +
> + if (virStrcpy(path, datastorePath, len) == NULL) {
> + VIR_FREE(path);
> + return NULL;
> + }
Why reimplement strdup here? Just use it:
path = strdup(datastorePath);
if (path == NULL) {
virReportOOMError();
return NULL;
}
> + return path;
> +}
> +
> +static virDomainPtr
> +vmwareDomainDefineXML(virConnectPtr conn, const char *xml)
> +{
> +
> + /* genrerate vmx file
> + * TODO : add vmware specific ProductVersion
No need to, as I'm going to change this in the VMX code.
> + */
> + vmx = esxVMX_FormatConfig(&ctx, driver->caps, vmdef,
> + esxVI_ProductVersion_ESX4x);
> + if (vmx == NULL) {
> + vmwareError(VIR_ERR_OPERATION_FAILED, "cannot create xml ");
Error message has trailing space and is not marked for translation.
> + goto cleanup;
> + }
> +
> + if (vmwareVmxPath(vmdef, &vmxPath) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("retrieve vmx path.. failed"));
> + goto cleanup;
> + }
> +
> + /* create vmx file */
> + int fd;
> +
> + if ((fd =
> + open(vmxPath, O_CREAT | O_WRONLY | O_TRUNC,
> + S_IRUSR | S_IWUSR)) == -1) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to open/create vmx file '%s'"), vmxPath);
> + goto cleanup;
> + }
> + if (safewrite(fd, vmx, strlen(vmx)) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to write vmx file '%s'"), vmxPath);
> + goto cleanup;
> + }
There's a helper function for this: virFileWriteStr
> + /* assign def */
> + if (!(vm = virDomainAssignDef(driver->caps,
> + &driver->domains, vmdef, false)))
> + goto cleanup;
> +
> + //* save vmxpath in domain object * //
> + if (VIR_ALLOC(pDomain) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + pDomain->vmxPath = strdup(vmxPath);
> + vm->privateData = (void *) pDomain;
> + vm->privateDataFreeFunc = vmwareDataFreeFunc;
Assing the privateDataFreeFunc in the caps init function and let the
virDomainObj handling code do this.
> +static int
> +vmwareDomainDestroy(virDomainPtr dom)
> +{
> + struct vmware_driver *driver = dom->conn->privateData;
> +
> + virDomainObjPtr vm;
> + const char *cmd[] = {
> + VMRUN, "-T", PROGRAM_SENTINAL, "stop",
> + PROGRAM_SENTINAL, "hard", NULL
> + };
> + int ret = -1;
> +
> + vmwareDriverLock(driver);
> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> + vmwareDriverUnlock(driver);
> +
> + if (!vm) {
> + vmwareError(VIR_ERR_INVALID_DOMAIN, "%s",
> + _("no domain with matching uuid"));
> + goto cleanup;
> + }
> +
> + vmwareSetSentinal(cmd, vmw_types[driver->type]);
> + vmwareSetSentinal(cmd, ((pvmwareDomain) vm->privateData)->vmxPath);
> + if (vm->state == VIR_DOMAIN_RUNNING) {
Here's a check missing if the domain is really destroyable, as in it's running.
> + }
> + if (virRun(cmd, NULL) < 0)
> + goto cleanup;
> + vm->def->id = -1;
> + vm->state = VIR_DOMAIN_SHUTOFF;
> +
> + if (!vm->persistent) {
> + virDomainRemoveInactive(&driver->domains, vm);
> + vm = NULL;
> + }
> +
> + ret = 0;
> + cleanup:
> + if (vm)
> + virDomainObjUnlock(vm);
> + return ret;
> +}
> +static int
> +vmwareDomainReboot(virDomainPtr dom, unsigned int flags ATTRIBUTE_UNUSED)
> +{
> + struct vmware_driver *driver = dom->conn->privateData;
> +
> + virDomainObjPtr vm;
> + const char *cmd[] = {
> + VMRUN, "-T", PROGRAM_SENTINAL,
> + "reset", PROGRAM_SENTINAL, NULL
> + };
> + int ret = -1;
> +
> + vmwareDriverLock(driver);
> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> + vmwareDriverUnlock(driver);
> +
> + if (!vm) {
> + vmwareError(VIR_ERR_INVALID_DOMAIN, "%s",
> + _("no domain with matching uuid"));
> + goto cleanup;
> + }
> +
> + vmwareSetSentinal(cmd, vmw_types[driver->type]);
> + vmwareSetSentinal(cmd, ((pvmwareDomain) vm->privateData)->vmxPath);
> +
> +
> + if (vm->state != VIR_DOMAIN_RUNNING) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("domain is not in running state"));
> + goto cleanup;
> + }
> +
> + if (virRun(cmd, NULL) < 0)
> + goto cleanup;
> +
> + vm->def->id = -1;
A running domain must have a unique ID >= 0.
> + vm->state = VIR_DOMAIN_RUNNING;
> + ret = 0;
> +
> + cleanup:
> + if (vm)
> + virDomainObjUnlock(vm);
> + return ret;
> +}
> +static virDomainPtr
> +vmwareDomainCreateXML(virConnectPtr conn, const char *xml,
> + unsigned int flags ATTRIBUTE_UNUSED)
> +{
> + virDomainPtr dom;
> +
> + dom = vmwareDomainDefineXML(conn, xml);
> + if (dom == NULL)
> + return NULL;
DomainCreateXML is supposed to create and start a trnasient domain.
Therefore you cannot call DomainDefineXML in it, as DomainDefineXML is
supposed to create a persistant domain.
> + if (vmwareDomainCreate(dom) < 0) {
> + virUnrefDomain(dom);
> + return NULL;
> + }
> +
> + return dom;
> +}
> diff --git a/src/vmware/vmware_driver.h b/src/vmware/vmware_driver.h
> new file mode 100644
> index 0000000..0996777
> --- /dev/null
> +++ b/src/vmware/vmware_driver.h
> +#ifndef VMWARE_DRIVER_H
> +# define VMWARE_DRIVER_H
> +
> +# include "internal.h"
No need to include internal.h here.
> +int vmwareRegister(void);
> +
> +#endif
> --
> 1.7.0.4
>
Some general comments:
- There are still some printf debugging lines in there that should be removed.
- strdup is used in any places without checking it's result for NULL
and reporting an OOM error if that's the case.
I applied and patch and did minimal testing with it on a VMware Player
and it works so far, no major problems.
As pointed out in this intial review there are still many (small) things to do.
I'll do some small refactroing to the VMX code and move it to src/util
in the next few days.
Matthias
More information about the libvir-list
mailing list