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

2010/12/2 Jean-Baptiste Rouault <jean-baptiste.rouault(a)diateam.net&gt;: virMutexDestroy(&driver->lock) is missing here. Use fgets(vmxPath, sizeof(vmxPath), fp) here instead. strlen returns size_t, not int. Also move size_t len to the block of other variables at the beginning of the function. 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. 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. 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); } conn is not unused here, so you don't need to mark it as such. 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. This will need the 3. argument for the extended virFileWriteStr function. 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. 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. Don't report and error here, vmwareParsePath does this already. Don't report and error here, vmwareParsePath does this already. The the return values of all this vmwareMakePath calls. Don't report and error here, vmwareMoveFile does this already. Don't report and error here, vmwareParsePath does this already. You should check the return value of vmwareMakePath Don't report and error here, vmwareMoveFile does this already. Don't report and error here, vmwareMoveFile does this already. Don't report and error here, vmwareMoveFile does this already. Don't report and error here, esxVMX_ParseConfig does this already. Don't report an error here, vmwareDomainDefineXML does this already. 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")); > + > +    /* 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; > +} 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