2010/12/14 Jean-Baptiste Rouault <jean-baptiste.rouault@xxxxxxxxxxx>:
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list