[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [Patch v2] Add VMware Workstation and Player driver



2010/12/2 Jean-Baptiste Rouault <jean-baptiste rouault 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]