[libvirt] [Patch v2] Add VMware Workstation and Player driver
Matthias Bolte
matthias.bolte at googlemail.com
Sun Dec 5 19:06:58 UTC 2010
2010/12/2 Jean-Baptiste Rouault <jean-baptiste.rouault at diateam.net>:
> ---
> configure.ac | 7 +
> include/libvirt/virterror.h | 1 +
> po/POTFILES.in | 2 +
> src/Makefile.am | 24 +-
> src/driver.h | 3 +-
> src/libvirt.c | 15 +
> src/util/virterror.c | 3 +
> src/vmware/vmware_conf.c | 480 +++++++++++++++
> src/vmware/vmware_conf.h | 82 +++
> src/vmware/vmware_driver.c | 1372 +++++++++++++++++++++++++++++++++++++++++++
> src/vmware/vmware_driver.h | 25 +
> 11 files changed, 2010 insertions(+), 4 deletions(-)
> create mode 100644 src/vmware/vmware_conf.c
> create mode 100644 src/vmware/vmware_conf.h
> create mode 100644 src/vmware/vmware_driver.c
> create mode 100644 src/vmware/vmware_driver.h
> diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c
> new file mode 100644
> index 0000000..5e873e1
> --- /dev/null
> +++ b/src/vmware/vmware_conf.c
> @@ -0,0 +1,480 @@
> +/*---------------------------------------------------------------------------*/
> +/* 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>
> +
> +#include "cpu/cpu.h"
> +#include "memory.h"
> +#include "nodeinfo.h"
> +#include "util/files.h"
> +#include "uuid.h"
> +#include "virterror_internal.h"
> +#include "../esx/esx_vmx.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;
virMutexDestroy(&driver->lock) is missing here.
> + virDomainObjListDeinit(&driver->domains);
> + virCapabilitiesFree(driver->caps);
> + VIR_FREE(driver);
> +}
> +
> +
> +int
> +vmwareLoadDomains(struct vmware_driver *driver)
> +{
> + FILE *fp;
> + virDomainDefPtr vmdef = NULL;
> + virDomainObjPtr vm = NULL;
> + char vmxPath[1024];
> + char * vmx = NULL;
> + vmwareDomainPtr pDomain;
> + char *directoryName = NULL;
> + char *fileName = NULL;
> + int ret = -1;
> + esxVMX_Context ctx;
> +
> + ctx.parseFileName = esxCopyVMXFileName;
> +
> + 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 (fgets(vmxPath, 1024, fp) == NULL) {
Use fgets(vmxPath, sizeof(vmxPath), fp) here instead.
> + if (feof(fp))
> + break;
> +
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to parse vmrun list output"));
> + goto cleanup;
> + }
> + if (vmxPath[0] != '/')
> + continue;
> +
> + /* remove trailing newline */
> + int len = strlen(vmxPath);
strlen returns size_t, not int. Also move size_t len to the block of
other variables at the beginning of the function.
> + if (len && vmxPath[len-1] == '\n')
> + vmxPath[len-1] = '\0';
> +
> + if (virFileReadAll(vmxPath, 10000, &vmx) == -1) {
> + perror("error reading vmx file");
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to read vmx file %s"), vmxPath);
> + goto cleanup;
> + }
> +
> + if ((vmdef =
> + esxVMX_ParseConfig(&ctx, driver->caps, vmx,
> + esxVI_ProductVersion_ESX4x)) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse config file"));
> + goto cleanup;
> + }
> +
> + if (!(vm = virDomainAssignDef(driver->caps,
> + &driver->domains, vmdef, false)))
> + goto cleanup;
> +
> + pDomain = vm->privateData;
> +
> + pDomain->vmxPath = strdup(vmxPath);
> + if (pDomain->vmxPath == NULL) {
> + virReportOOMError();
> + goto no_memory;
Just goto cleanup here, as you already reported the OOM error. Now you
can remove the no_memory label and avoid the jump backwards to the
cleanup label.
> + }
> +
> + vmwareDomainConfigDisplay(pDomain, vmdef);
> +
> + //vmrun list only reports running vms
> + vm->state = VIR_DOMAIN_RUNNING;
> + vm->def->id = driver->nextvmid++;
> + vm->persistent = 1;
> +
> + virDomainObjUnlock(vm);
> +
> + vmdef = NULL;
> + vm = NULL;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + virDomainDefFree(vmdef);
> + VIR_FORCE_FCLOSE(fp);
> + VIR_FREE(directoryName);
> + VIR_FREE(fileName);
> + VIR_FREE(vmx);
> + if (vm)
> + virDomainObjUnref(vm);
> + return ret;
> +
> +no_memory:
> + virReportOOMError();
> + goto cleanup;
> +}
> +
> +
> +int
> +vmwareExtractVersion(struct vmware_driver *driver)
> +{
> + unsigned long version = 0;
> + FILE *fp = NULL;
> + char str[50];
> + char *tmp;
> + int ret = -1;
> +
> + if (driver->type == TYPE_PLAYER) {
> + if ((fp = popen("vmplayer -v 2>/dev/null", "r")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", _("popen failed"));
> + goto cleanup;
> + }
> + if (!feof(fp) && fgets(str, 49, fp)) {
> + if ((tmp = STRSKIP(str, "VMware Player ")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("vmplayer -v parsing error"));
> + goto cleanup;
> + }
> + if (virParseVersionString(tmp, &version) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("version parsing error"));
> + goto cleanup;
> + }
> + driver->version = version;
> + ret = 0;
> + } else {
> + 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"));
> + goto cleanup;
> + }
> + if (!feof(fp) && fgets(str, 49, fp)) {
> + if ((tmp = STRSKIP(str, "VMware Workstation ")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("vmware -v parsing error"));
> + goto cleanup;
> + }
> + if (virParseVersionString(tmp, &version) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("version parsing error"));
> + goto cleanup;
> + }
> + driver->version = version;
> + ret = 0;
> + } else {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("vmware -v reading error"));
> + }
> + }
> +
> +cleanup:
> + VIR_FORCE_FCLOSE(fp);
> + return ret;
> +}
> +
> +
> +int
> +vmwareParsePath(char *path, char **directory, char **filename)
> +{
> + char *separator;
> +
> + separator = strrchr(path, '/');
> +
> + if (separator != NULL) {
> + *separator++ = '\0';
> +
> + if (*separator == '\0') {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("path '%s' doesn't reference a file"), path);
> + return -1;
> + }
> +
> + if ((*directory = strdup(path)) == NULL)
> + goto no_memory;
> + if ((*filename = strdup(separator)) == NULL)
> + goto no_memory;
> +
> + } else {
> + if ((*filename = strdup(separator)) == NULL)
> + goto no_memory;
> + }
> +
> + return 0;
> +
> +no_memory:
> + virReportOOMError();
> + return -1;
> +}
> +
> +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;
> +}
> +
> +int
> +vmwareMoveFile(char *srcFile, char *dstFile)
> +{
> + const char *cmdmv[] =
> + { "mv", PROGRAM_SENTINAL, PROGRAM_SENTINAL, NULL };
> +
> + if (!virFileExists(srcFile)) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, _("file %s does not exist"),
> + srcFile);
> + return -1;
> + }
> +
> + if (STREQ(srcFile, dstFile))
> + return 0;
> +
> + vmwareSetSentinal(cmdmv, srcFile);
> + vmwareSetSentinal(cmdmv, dstFile);
> + if (virRun(cmdmv, NULL) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to move file to %s "), dstFile);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +int
> +vmwareMakePath(char *srcDir, char *srcName, char *srcExt, char **outpath)
> +{
> + if (virAsprintf(outpath, "%s/%s.%s", srcDir, srcName, srcExt) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> + return 0;
> +}
> +
> diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
> new file mode 100644
> index 0000000..5615bef
> --- /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 /session, then they typoed, so tell them correct path */
> + if (conn->uri->path == NULL || STRNEQ(conn->uri->path, "/session")) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + ("unexpected VMware URI path '%s', try vmwareplayer:///session or vmwarews:///session"),
> + conn->uri->path);
Should be _() instead of (). Add vmwareError to the list of
msg_gen_function's in cfg.mk to let "make syntax-check" verify this.
Also you might feed a NULL string for %s, that's not save. Use
NULLSTR(conn->uri->path) here in the vmwareError call, or reorder the
testing logic.
> + return VIR_DRV_OPEN_ERROR;
> + }
> + }
> +
> + /* We now know the URI is definitely for this driver, so beyond
> + * here, don't return DECLINED, always use ERROR */
You should probably check early if vmrun is available at all, For
example like this.
char *vmrun = virFindFileInPath(VMRUN);
if (vmrun == NULL) {
vmwareError(VIR_ERR_INTERNAL_ERROR,
_("%s utility is missing"), VMRUN);
return VIR_DRV_OPEN_ERROR;
} else {
VIR_FREE(vmrun);
}
> + if (VIR_ALLOC(driver) < 0) {
> + virReportOOMError();
> + return VIR_DRV_OPEN_ERROR;
> + }
> +static const char *
> +vmwareGetType(virConnectPtr conn ATTRIBUTE_UNUSED)
conn is not unused here, so you don't need to mark it as such.
> +{
> + struct vmware_driver *driver = conn->privateData;
> + int type;
> +
> + vmwareDriverLock(driver);
> + type = driver->type;
> + vmwareDriverUnlock(driver);
> + return type == TYPE_PLAYER ? "vmware player" : "vmware workstation";
> +}
> +static virDomainPtr
> +vmwareDomainDefineXML(virConnectPtr conn, const char *xml)
> +{
> + struct vmware_driver *driver = conn->privateData;
> + virDomainDefPtr vmdef = NULL;
> + virDomainObjPtr vm = NULL;
> + virDomainPtr dom = NULL;
> + char *vmx = NULL;
> + char *directoryName = NULL;
> + char *fileName = NULL;
> + char *vmxPath = NULL;
> + vmwareDomainPtr pDomain = NULL;
> + esxVMX_Context ctx;
> +
> + ctx.formatFileName = esxCopyVMXFileName;
> +
> + vmwareDriverLock(driver);
> + if ((vmdef = virDomainDefParseString(driver->caps, xml,
> + VIR_DOMAIN_XML_INACTIVE)) == NULL)
> + goto cleanup;
> +
> + vm = virDomainFindByName(&driver->domains, vmdef->name);
> + if (vm) {
> + vmwareError(VIR_ERR_OPERATION_FAILED,
> + _("Already an VMWARE VM active with the id '%s'"),
> + vmdef->name);
> + goto cleanup;
> + }
> +
> + /* generate vmx file */
> + vmx = esxVMX_FormatConfig(&ctx, driver->caps, vmdef,
> + esxVI_ProductVersion_ESX4x);
> + if (vmx == NULL) {
> + vmwareError(VIR_ERR_OPERATION_FAILED, _("cannot create xml"));
Don't report your own error here, esxVMX_FormatConfig did this already
when it returns NULL. Your error message will overwrite the more
detailed error message reported by esxVMX_FormatConfig.
> + goto cleanup;
> + }
> +
> + if (vmwareVmxPath(vmdef, &vmxPath) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("retrieve vmx path.. failed"));
> + goto cleanup;
> + }
> +
> + /* create vmx file */
> + if (virFileWriteStr(vmxPath, vmx) < 0) {
This will need the 3. argument for the extended virFileWriteStr function.
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to write vmx file '%s'"), vmxPath);
> + goto cleanup;
> + }
> +static int
> +vmwareDomainSuspend(virDomainPtr dom)
> +{
> + struct vmware_driver *driver = dom->conn->privateData;
> +
> + virDomainObjPtr vm;
> + const char *cmd[] = {
> + VMRUN, "-T", PROGRAM_SENTINAL, "pause",
> + PROGRAM_SENTINAL, NULL
> + };
> + int ret = -1;
> +
> + if (driver->type == TYPE_PLAYER) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("vmplayer does not support libvirt suspend/resume"
> + " (vmware pause/unpause) operation "));
> + return ret;
> + }
> +
> + 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]);
All access to the driver struct is made with the driver lock held,
expect this one. Actually accessing driver->type unlocked should be
okay, as driver->type is set in the open function and then only read.
In some functions you take the lock to access driver->type in some you
don't. Choose one way and then be consistent.
> +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
> + };
virDomainReboot is meant to reboot the guest OS as in executing the
reboot command from inside the guest OS. It's not meant to do a
hardware level reset as in pressing the reset button of an actual
computer.
I'm not sure "vmrun reset" is the right thing here.
> +static int
> +vmwareDomainSave(virDomainPtr dom, const char *path)
> +{
> + struct vmware_driver *driver = dom->conn->privateData;
> + virDomainObjPtr vm;
> + const char *cmdsuspend[] = {
> + VMRUN, "-T", PROGRAM_SENTINAL,
> + "suspend", PROGRAM_SENTINAL, NULL
> + };
> + int ret = -1;
> + char *fDirectoryName = NULL;
> + char *fFileName = NULL;
> + char *tDirectoryName = NULL;
> + char *tFileName = NULL;
> + char *copyPath = NULL;
> + char *copyvmxPath = NULL;
> + char *fvmss = NULL;
> + char *tvmss = NULL;
> + char *fvmem = NULL;
> + char *tvmem = NULL;
> + char *fvmx = NULL;
> + char *tvmx = NULL;
> +
> + 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;
> + }
> +
> + /* vmware suspend */
> + vmwareSetSentinal(cmdsuspend, vmw_types[driver->type]);
> + vmwareSetSentinal(cmdsuspend,
> + ((vmwareDomainPtr) 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(cmdsuspend, NULL) < 0)
> + goto cleanup;
> +
> + /* create files path */
> + copyvmxPath = strdup(((vmwareDomainPtr) vm->privateData)->vmxPath);
> + if(copyvmxPath == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if((copyPath = strdup(path)) == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (vmwareParsePath(copyvmxPath, &fDirectoryName, &fFileName) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to parse vmxPath"));
Don't report and error here, vmwareParsePath does this already.
> + goto cleanup;
> + }
> + if (vmwareParsePath(copyPath, &tDirectoryName, &tFileName) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to parse path"));
Don't report and error here, vmwareParsePath does this already.
> + goto cleanup;
> + }
> +
> + /* dir */
> + if (virFileMakePath(tDirectoryName) != 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s", _("make path error"));
> + goto cleanup;
> + }
> +
> + vmwareMakePath(fDirectoryName, vm->def->name, (char *) "vmss", &fvmss);
> + vmwareMakePath(tDirectoryName, vm->def->name, (char *) "vmss", &tvmss);
> + vmwareMakePath(fDirectoryName, vm->def->name, (char *) "vmem", &fvmem);
> + vmwareMakePath(tDirectoryName, vm->def->name, (char *) "vmem", &tvmem);
> + vmwareMakePath(fDirectoryName, vm->def->name, (char *) "vmx", &fvmx);
> + vmwareMakePath(tDirectoryName, vm->def->name, (char *) "vmx", &tvmx);
The the return values of all this vmwareMakePath calls.
> + /* create linker file */
> + int fd;
> +
> + if ((fd =
> + open(path, O_CREAT | O_WRONLY | O_TRUNC,
> + S_IRUSR | S_IWUSR)) == -1) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to open linker file '%s'"), path);
> + goto cleanup;
> + }
> + if (safewrite
> + (fd, ((vmwareDomainPtr) vm->privateData)->vmxPath,
> + strlen(((vmwareDomainPtr) vm->privateData)->vmxPath)) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to create linker file"));
> + goto cleanup;
> + }
> +
> + /* we want to let saved VM inside default directory */
> + if (STREQ(fDirectoryName, tDirectoryName))
> + goto end;
> +
> + /* move {vmx,vmss,vmem} files */
> + if ((vmwareMoveFile(fvmss, tvmss) < 0)
> + || (vmwareMoveFile(fvmem, tvmem) < 0)
> + || (vmwareMoveFile(fvmx, tvmx) < 0)
> + ) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to move file"));
Don't report and error here, vmwareMoveFile does this already.
> + goto cleanup;
> + }
> +
> + end:
> + vm->def->id = -1;
> + vm->state = VIR_DOMAIN_SHUTOFF;
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(fDirectoryName);
> + VIR_FREE(fFileName);
> + VIR_FREE(tDirectoryName);
> + VIR_FREE(tFileName);
> + VIR_FREE(copyPath);
> + VIR_FREE(copyvmxPath);
> +
> + VIR_FREE(fvmss);
> + VIR_FREE(tvmss);
> + VIR_FREE(fvmem);
> + VIR_FREE(tvmem);
> + VIR_FREE(fvmx);
> + VIR_FREE(tvmx);
> + if (vm)
> + virDomainObjUnlock(vm);
> + return ret;
> +}
> +
> +static int
> +vmwareDomainRestore(virConnectPtr conn, const char *path)
> +{
> + struct vmware_driver *driver = conn->privateData;
> + virDomainDefPtr vmdef = NULL;
> + virDomainPtr dom = NULL;
> + virDomainObjPtr vm = NULL;
> + int ret = -1;
> + char *vmxPath = NULL;
> + char *copypath = NULL;
> + char *copyvmxPath = NULL;
> + char *tDirectoryName = NULL;
> + char *tFileName = NULL;
> + char *fDirectoryName = NULL;
> + char *fFileName = NULL;
> + char *vmx = NULL;
> + char *xml = NULL;
> + char *sep = NULL;
> + char *baseVmss;
> + char *fvmss = NULL;
> + char *tvmss = NULL;
> + char *fvmem = NULL;
> + char *tvmem = NULL;
> + char *fvmx = NULL;
> + char *tvmx = NULL;
> + esxVMX_Context ctx;
> +
> + ctx.parseFileName = esxCopyVMXFileName;
> +
> + if (!virFileExists(path)) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("file %s does not exist"), path);
> + goto cleanup;
> + }
> + if (virFileHasSuffix(path, ".vmx")) { //we want to restore a vm saved in its default directory
> + if((tvmx = strdup(path)) == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if((copypath = strdup(path)) == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (vmwareParsePath(copypath, &fDirectoryName, &fFileName) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to parse path"));
Don't report and error here, vmwareParsePath does this already.
> + goto cleanup;
> + }
> + sep = strrchr(fFileName, '.');
> + if (sep != NULL)
> + *sep = '\0';
> + vmwareMakePath(fFileName, fFileName, (char *) "vmss", &fvmss);
You should check the return value of vmwareMakePath
> + baseVmss = basename(fvmss);
> + } else { //we want to restore a vm saved elsewhere
> + if (virFileReadAll(path, 1024, &vmxPath) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to read vmxPath"));
> + goto cleanup;
> + }
> +
> + if (!virFileHasSuffix(vmxPath, ".vmx")) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("%s is not a .vmx file"), vmxPath);
> + goto cleanup;
> + }
> +
> + /* move files */
> + if((copyvmxPath = strdup(vmxPath)) == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if((copypath = strdup(path)) == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (vmwareParsePath(copypath, &fDirectoryName, &fFileName) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to parse path"));
Don't report and error here, vmwareMoveFile does this already.
> + goto cleanup;
> + }
> + if (vmwareParsePath(copyvmxPath, &tDirectoryName, &tFileName) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to parse vmxPath"));
Don't report and error here, vmwareMoveFile does this already.
> + goto cleanup;
> + }
> +
> + sep = strrchr(tFileName, '.');
> + if (sep != NULL)
> + *sep = '\0';
> +
> + if (virFileMakePath(tDirectoryName) != 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("make path error"));
> + goto cleanup;
> + }
> +
> + vmwareMakePath(fDirectoryName, tFileName, (char *) "vmss", &fvmss);
> + vmwareMakePath(tDirectoryName, tFileName, (char *) "vmss", &tvmss);
> + vmwareMakePath(fDirectoryName, tFileName, (char *) "vmem", &fvmem);
> + vmwareMakePath(tDirectoryName, tFileName, (char *) "vmem", &tvmem);
> + vmwareMakePath(fDirectoryName, tFileName, (char *) "vmx", &fvmx);
> + vmwareMakePath(tDirectoryName, tFileName, (char *) "vmx", &tvmx);
> +
> + if ((vmwareMoveFile(fvmss, tvmss) < 0)
> + || (vmwareMoveFile(fvmem, tvmem) < 0)
> + || (vmwareMoveFile(fvmx, tvmx) < 0)
> + ) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to move files"));
Don't report and error here, vmwareMoveFile does this already.
> + goto cleanup;
> + }
> +
> + baseVmss = basename(tvmss);
> + }
> +
> + if (virFileReadAll(tvmx, 10000, &vmx) == -1) {
> + perror("error reading vmx file");
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to read vmx file %s"), tvmx);
> + goto cleanup;
> + }
> +
> + vmwareDriverLock(driver);
> + if ((vmdef =
> + esxVMX_ParseConfig(&ctx, driver->caps, vmx,
> + esxVI_ProductVersion_ESX4x)) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse config file"));
Don't report and error here, esxVMX_ParseConfig does this already.
> + goto cleanup;
> + }
> + vmwareDriverUnlock(driver);
> +
> + xml = virDomainDefFormat(vmdef, VIR_DOMAIN_XML_SECURE);
> +
> + if ((dom = vmwareDomainDefineXML(conn, xml)) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to define domain"));
Don't report an error here, vmwareDomainDefineXML does this already.
> + goto cleanup;
> + }
> +
> + /* esxVMX_ParseConfig don't care about vmx checkpoint property for now, so we add it here
> + * TODO
> + */
> + FILE *pFile = NULL;
> +
> + if ((pFile = fopen(tvmx, "a+")) == NULL) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to reopen vmx file"));
> + goto cleanup;
> + }
> +
> + fputs("checkpoint.vmState = \"", pFile);
> + fputs(baseVmss, pFile);
> + fputs("\"", pFile);
> + fclose(pFile);
> +
> + vmwareDriverLock(driver);
> + vm = virDomainFindByName(&driver->domains, dom->name);
> +
> + if (!vm) {
> + vmwareError(VIR_ERR_INVALID_DOMAIN, "%s",
> + _("no domain with matching id"));
> + goto cleanup;
> + }
> +
> + /* start */
> + if (vmwareStartVM(driver, vm) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to create domain"));
Don't report an error here, vmwareStartVM does this already.
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(vmxPath);
> + VIR_FREE(copypath);
> + VIR_FREE(copyvmxPath);
> + VIR_FREE(tDirectoryName);
> + VIR_FREE(tFileName);
> + VIR_FREE(fDirectoryName);
> + VIR_FREE(fFileName);
> + VIR_FREE(vmx);
> + VIR_FREE(xml);
> +
> + VIR_FREE(fvmss);
> + VIR_FREE(tvmss);
> + VIR_FREE(fvmem);
> + VIR_FREE(tvmem);
> + VIR_FREE(fvmx);
> + VIR_FREE(tvmx);
> +
> + if (dom)
> + virUnrefDomain(dom);
> + if (vm)
> + virDomainObjUnlock(vm);
> + vmwareDriverUnlock(driver);
> + return ret;
> +}
> +
> +static virDomainPtr
> +vmwareDomainCreateXML(virConnectPtr conn, const char *xml,
> + unsigned int flags ATTRIBUTE_UNUSED)
> +{
> + struct vmware_driver *driver = conn->privateData;
> + virDomainDefPtr vmdef = NULL;
> + virDomainObjPtr vm = NULL;
> + virDomainPtr dom = NULL;
> + char *vmx = NULL;
> + char *vmxPath = NULL;
> + vmwareDomainPtr pDomain = NULL;
> + esxVMX_Context ctx;
> +
> + ctx.formatFileName = esxCopyVMXFileName;
> +
> + vmwareDriverLock(driver);
> +
> + if ((vmdef = virDomainDefParseString(driver->caps, xml,
> + VIR_DOMAIN_XML_INACTIVE)) == NULL)
> + goto cleanup;
> +
> + vm = virDomainFindByName(&driver->domains, vmdef->name);
> + if (vm) {
> + vmwareError(VIR_ERR_OPERATION_FAILED,
> + _("Already an VMWARE VM active with the id '%s'"),
> + vmdef->name);
> + goto cleanup;
> + }
> +
> + /* generate vmx file */
> + vmx = esxVMX_FormatConfig(&ctx, driver->caps, vmdef,
> + esxVI_ProductVersion_ESX4x);
> + if (vmx == NULL) {
> + vmwareError(VIR_ERR_OPERATION_FAILED, _("cannot create xml"));
> + goto cleanup;
> + }
> +
> + if (vmwareVmxPath(vmdef, &vmxPath) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("retrieve vmx path.. failed"));
> + goto cleanup;
> + }
> +
> + /* create vmx file */
> + if (virFileWriteStr(vmxPath, vmx) < 0) {
> + vmwareError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to write vmx file '%s'"), vmxPath);
> + goto cleanup;
> + }
> +
> + /* assign def */
> + if (!(vm = virDomainAssignDef(driver->caps,
> + &driver->domains, vmdef, false)))
> + goto cleanup;
> +
> + pDomain = vm->privateData;
> + pDomain->vmxPath = strdup(vmxPath);
> +
> + vmwareDomainConfigDisplay(pDomain, vmdef);
> + vmdef = NULL;
> +
> + if (vmwareStartVM(driver, vm) < 0) {
> + virDomainRemoveInactive(&driver->domains, vm);
> + vm = NULL;
> + goto cleanup;
> + }
> +
> + dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> + if (dom)
> + dom->id = vm->def->id;
> +
> +cleanup:
> + virDomainDefFree(vmdef);
> + VIR_FREE(vmx);
> + VIR_FREE(vmxPath);
> + if(vm)
> + virDomainObjUnlock(vm);
> + vmwareDriverUnlock(driver);
> + return dom;
> +}
> +static int
> +vmwareDomainUndefine(virDomainPtr dom)
> +{
> + struct vmware_driver *driver = dom->conn->privateData;
> + virDomainObjPtr vm;
> + int ret = -1;
> +
> + vmwareDriverLock(driver);
> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +
> + if (!vm) {
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> + virUUIDFormat(dom->uuid, uuidstr);
> + vmwareError(VIR_ERR_NO_DOMAIN,
> + _("no domain with matching uuid '%s'"), uuidstr);
> + goto cleanup;
> + }
> +
> + if (virDomainObjIsActive(vm)) {
> + vmwareError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("cannot delete active domain"));
s/delete/undefine/
At the end of my review I noticed that you overwrite already reported
errors in many places. I only commented on some of them so you should
check for this in the whole driver.
I think the next version of this driver will be ready to be added to
the official codebase :)
Matthias
More information about the libvir-list
mailing list