Re: [libvirt] [PATCH] Support for virtualbox version 3.1

Well, apart from the new auto generated files for version 3.1 support a huge part of the patch affects vbox_tmpl.c, ~7700 lines changed. Most of that are pure indentation level changes, due to inverting the logic of some common error checks. Applying the patch and creating a new patch using git diff -b to ignore pure whitespace changes results in ~2200 lines changed. And even most of this 2200 lines are due to the wrapping of some common code patterns into macros; mostly free/release calls. The actual functional change of this patch is fairly small. You should have split this at least into two separate patches.

[...] > + return NULL; > + } > + > + if (storageBus == StorageBus_IDE) { > + name[0] = 'h'; > + name[1] = 'd'; > + } else if ( (storageBus == StorageBus_SATA) > + || (storageBus == StorageBus_SCSI)) { > + name[0] = 's'; > + name[1] = 'd'; > + } else if (storageBus == StorageBus_Floppy) { > + name[0] = 'f'; > + name[1] = 'd'; > + } You're missing an else branch here to handle the case of the storage bus being none of the checked values.

> + if (len == 4) { > + name[2] = (char)(97 + total); If total is out-of-range len will also be 4, because 4 is the default value and you have no out-or-range checks to catch such a situation. So you're going to create a bogus name then.

> + } else if (len == 5) { > + name[2] = (char)(96 + (total / 26)); > + name[3] = (char)(97 + (total % 26)); > + } else if (len == 6) { > + name[2] = (char)(96 + (total / 26*26)); > + name[3] = (char)(96 + ((total % (26*26)) / 26)); > + name[4] = (char)(97 + ((total % (26*26)) % 26)); > + } You should use virDiskNameToIndex() in vboxGetDeviceDetails() (see below). So you should use the matching inverse function here. I added esxVMX_IndexToDiskName() for the ESX driver, because libvirt doesn't have this function yet. Maybe I should post a patch to add it to src/util/util.[ch].

Actually, it seems you do the correct inverse operation to virDiskNameToIndex() with this code. > + name[len - 1] = '\0'; > + DEBUG("name=%s, len=%d, total=%d, storageBus=%u, deviceInst=%d, " > + "devicePort=%d deviceSlot=%d, maxPortPerInst=%u > maxSlotPerPort=%u", + name, len, total, storageBus, deviceInst, > devicePort, + deviceSlot, maxPortPerInst, maxSlotPerPort); > + return name; > +} > + > +/** > + * function to get the StorageBus, Port number > + * and Device number for the given devicename > + * e.g: hda has StorageBus = IDE, port = 0, > + * device = 0 > + * > + * Limitation: only name till 4 chars supported > + * i.e from hda till hdzz > + * > + * @returns true on Success, false on failure. > + * @param deviceName Input device name > + * @param aMaxPortPerInst Input array of max port per device > instance + * @param aMaxSlotPerPort Input array of max slot per > device port + * @param storageBus Input storage bus type > + * @param deviceInst Output device instance number > + * @param devicePort Output port number > + * @param deviceSlot Output slot number > + * > + */ > +static bool vboxGetDeviceDetails(const char *deviceName, > + PRUint32 *aMaxPortPerInst, > + PRUint32 *aMaxSlotPerPort, > + PRUint32 storageBus, > + PRInt32 *deviceInst, > + PRInt32 *devicePort, > + PRInt32 *deviceSlot) { > + int len = 0; > + int total = 0; > + PRUint32 maxPortPerInst = 0; > + PRUint32 maxSlotPerPort = 0; > + > + if ( !deviceName > + || !storageBus > + || !deviceInst > + || !devicePort > + || !deviceSlot > + || !aMaxPortPerInst > + || !aMaxSlotPerPort) > + return false; > + > + len = strlen(deviceName); > + > + /* support till hdzz only so 4 chars */ > + if (len > 4) > + return false; > + > + maxPortPerInst = aMaxPortPerInst[storageBus]; > + maxSlotPerPort = aMaxSlotPerPort[storageBus]; > + > + if ( !maxPortPerInst > + || !maxSlotPerPort) > + return false; > + > + if (len == 3) { > + total = (deviceName[2] - 97); > + } else if (len == 4) { > + total = ((deviceName[2] - 96) * 26) + (deviceName[3] - 97); > + } You assume that you got sane input here. This code is going to fail due to char underflow if the input is 'hdA' instead of 'hda'. You should use the more robust virDiskNameToIndex() here.

> + *deviceInst = total / (maxPortPerInst * maxSlotPerPort); > + *devicePort = (total % (maxPortPerInst * maxSlotPerPort)) / > maxSlotPerPort; + *deviceSlot = (total % (maxPortPerInst * > maxSlotPerPort)) % maxSlotPerPort; + > + DEBUG("name=%s, len=%d, total=%d, storageBus=%u, deviceInst=%d, " > + "devicePort=%d deviceSlot=%d, maxPortPerInst=%u > maxSlotPerPort=%u", + deviceName, len, total, storageBus, > *deviceInst, *devicePort, + *deviceSlot, maxPortPerInst, > maxSlotPerPort); > + > + return true; > +} [...] > +/** > + * Converts Utf-16 string to int > + */ > +static int PRUnicharToInt(PRUnichar *strUtf16) { > + char *strUtf8 = NULL; > + int ret = 0; > + > + if (!strUtf16) > + return -1; > + > + g_pVBoxGlobalData->pFuncs->pfnUtf16ToUtf8(strUtf16, &strUtf8); > + if (!strUtf8) > + return -1; > + > + ret = atoi(strUtf8); > + g_pVBoxGlobalData->pFuncs->pfnUtf8Free(strUtf8); > + > + return ret; > +} > + > +/** > + * Converts int to Utf-16 string > + */ > +static PRUnichar *PRUnicharFromInt(int n) { > + PRUnichar *strUtf16 = NULL; > + char c, s[24] = {}; > + int sign, i = 0, j = 0; > + > + if ((sign = n) < 0) > + n = -n; > + do { > + s[i++] = n % 10 + '0'; > + } while ((n /= 10) > 0); > + if (sign < 0) > + s[i++] = '-'; > + s[i] = '\0'; > + for (j = 0; j < i; j++, i--) { > + c = s[j]; > + s[j] = s[i]; > + s[i] = c; > + } > + > + g_pVBoxGlobalData->pFuncs->pfnUtf8ToUtf16(s, &strUtf16); > + > + return strUtf16; > +} I don't understand why you do this in such a complex way. What about snprintf?

PRUnichar *strUtf16 = NULL; char s[24]; snprintf(s, sizeof(s), "%d", n); g_pVBoxGlobalData->pFuncs->pfnUtf8ToUtf16(s, &strUtf16); return strUtf16; > +#endif /* VBOX_API_VERSION >= 3001 */ > + > #endif /* !(VBOX_API_VERSION == 2002) */ > > static virCapsPtr vboxCapsInit(void) { ACK, apart from the minor issues in vboxGenerateMediumName() and vboxGetDeviceDetails(). Matthias