[libvirt] [Patch v4] Add VMware Workstation and Player driver

[Matthias Bolte]

2010/12/14 Jean-Baptiste Rouault <jean-baptiste.rouault at diateam.net>:
> ---

> diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c
> new file mode 100644
> index 0000000..a3a0689
> --- /dev/null
> +++ b/src/vmware/vmware_conf.c

> +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)

At this point strdup(path) succeeded and this leaks *directory here.
Adding VIR_FREE(*directory) here will fix it.

> +            goto no_memory;
> +
> +    } else {
> +        if ((*filename = strdup(separator)) == NULL)

separator is NULL here. I think you want to duplicate path instead.

> +            goto no_memory;
> +    }
> +
> +    return 0;
> +
> +no_memory:
> +    virReportOOMError();
> +    return -1;
> +}

> diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
> new file mode 100644
> index 0000000..5967c68
> --- /dev/null
> +++ b/src/vmware/vmware_driver.c

> +static int
> +vmwareStartVM(struct vmware_driver *driver, virDomainObjPtr vm)
> +{
> +    const char *cmd[] = {
> +        VMRUN, "-T", PROGRAM_SENTINAL, "start",
> +        PROGRAM_SENTINAL, PROGRAM_SENTINAL, NULL
> +    };
> +    const char *vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath;
> +
> +    if (vm->state != 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)
> +        vmwareSetSentinal(cmd, NOGUI);
> +    else
> +        vmwareSetSentinal(cmd, NULL);
> +
> +    if (virRun(cmd, NULL) < 0) {
> +        vmwareError(VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VMRUN);
> +        return -1;
> +    }
> +
> +    if ((vm->def->id = vmwareExtractPid(vmxPath)) < 0) {
> +        vmwareStopVM(driver, vm);
> +        return -1;
> +    }

Is this a possible race here? You start the VM and then try to read
the PID from the log. Is it possible that you try to read the PID
before the VM process has written it to the log?

> +    vm->state = VIR_DOMAIN_RUNNING;
> +
> +    return 0;
> +}


> +static int
> +vmwareDomainSave(virDomainPtr dom, const char *path)
> +{

> +    /* 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)) {
> +        goto cleanup;
> +    }

I think moving the files away is a bad idea, especially the .vmx file.
They should be copied.


> +static int
> +vmwareDomainRestore(virConnectPtr conn, const char *path)
> +{

> +        if ((vmwareMoveFile(fvmss, tvmss) < 0)
> +            || (vmwareMoveFile(fvmem, tvmem) < 0)
> +            || (vmwareMoveFile(fvmx, tvmx) < 0)) {
> +            goto cleanup;
> +        }

The files should be copied here too, as that would be in line with
what the QEMU and Xen drivers too. They keep the savefile intact after
a restore.

> +        baseVmss = basename(tvmss);
> +    }
> +
> +    if (virFileReadAll(tvmx, 10000, &vmx) < 0)
> +        goto cleanup;
> +
> +    vmwareDriverLock(driver);
> +    if ((vmdef =
> +         esxVMX_ParseConfig(&ctx, driver->caps, vmx,
> +                            esxVI_ProductVersion_ESX4x)) == NULL) {
> +        goto cleanup;
> +    }
> +    vmwareDriverUnlock(driver);
> +
> +    xml = virDomainDefFormat(vmdef, VIR_DOMAIN_XML_SECURE);
> +
> +    if ((dom = vmwareDomainDefineXML(conn, xml)) == NULL)
> +        goto cleanup;
> +
> +    /* esxVMX_ParseConfig doesn't care about vmx checkpoint property for
> +     * now, so we add it here
> +     * TODO
> +     */
> +
> +    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);

During testing I hit a case where the VMware Player GUI complained
about having this entry twice in the .vmx file. We might need to give
the .vmx handling code a function that allows to override/add entries
to make this more robust.


As you suggested on IRC, you could remove the save/restore functions
from the initial patch and put it in a followup patch.

Besides save/restore the last issues with v4 are in vmwareParsePath as
mentioned there.

So a v5 with the two problems in vmwareParsePath fixed and without
save/restore will get my ACK :)

Matthias