2010/11/15 Jean-Baptiste Rouault <jean-baptiste.rouault(a)diateam.net&gt;: Typo here, with_wmware=yes should be with_vmware=yes IMHO "VMware" as name for this driver is too generic, but I don't have a better alternative, so we'll probably just stick with it. s/Vmware/VMware/ One call to virDriverLoadModule for each driver is enough. So this driver is going to be Linux-only. Any intention to make it work on Windows too? I don't think that you can use utsname.machine to determine the supported architectures, as you can install and run a 32bit VMware Player/Workstation on a 64bit system and only use 32bit guests. Or am I wrong here? I think you need to query this information somehow for the installed VMware Player/Workstation. Also the sizeof(int) wordsize detection seems wrong to me. I found that LXC, OpenVZ and Phyp driver include the same line. The driver uses privateData in virDomainObj, but you don't set caps->privateDataAllocFunc and caps->privateDataFreeFunc. Therefore, you leak the privateData memory. See the QEMU and LXC driver how to use this. Remove the empty lines between the variables. fscanf to an 100 char buffer is asking for a buffer overflow. Using fgets with a 1k sized buffer might be a better approach here. Check the result of strdup for OOM (pDomain->vmxPath == NULL). Don't allocate a virDomainObj on your own. Also don't make up the domain name from the VMX filename and don't generate a new UUID. All this information should be taken from the content of the VMX file. Use the VMX file parser to obtain a virDomainDef from the VMX file and add a new domain to the driver->domains list using virDomainAssignDef. All error messages in this function miss the _ before the () around the message. Also fclose(fp) is missing and we are currently deprecating fclose in libvirt, use the VIR_FCLOSE macro instead. In this function _() is split on multiple lines in an unusual way _\n(). I suggest to stick to the usual way of _(). z seems unused, remove it? I suggest renmaing pvmwareDomain to vmwareDomainPtr to stick to the libvirt naming patterns. Isn't this more in the sense of /session than /system? As /system means that there is one system-wide management instance, but VMware Player/WS operate at a per user level, don't they? Don't compare to false, use the ! operator instead. Parsing a non-existing number from the domain name will not result in a proper ID. libvirt requires that the domain ID is unique and >= 0 for non-stopped domains and -1 for stopped domains. Use ATTRIBUTE_UNUSED instead of this. Why reimplement strdup here? Just use it: path = strdup(datastorePath); if (path == NULL) { virReportOOMError(); return NULL; } No need to, as I'm going to change this in the VMX code. Error message has trailing space and is not marked for translation. There's a helper function for this: virFileWriteStr Assing the privateDataFreeFunc in the caps init function and let the virDomainObj handling code do this. Here's a check missing if the domain is really destroyable, as in it's running. A running domain must have a unique ID >= 0. DomainCreateXML is supposed to create and start a trnasient domain. Therefore you cannot call DomainDefineXML in it, as DomainDefineXML is supposed to create a persistant domain. No need to include internal.h here. Some general comments: - There are still some printf debugging lines in there that should be removed. - strdup is used in any places without checking it's result for NULL and reporting an OOM error if that's the case. I applied and patch and did minimal testing with it on a VMware Player and it works so far, no major problems. As pointed out in this intial review there are still many (small) things to do. I'll do some small refactroing to the VMX code and move it to src/util in the next few days. Matthias