[libvirt] [PATCH] Support for virtualbox version 3.1
Hi All, I have just added support for VirtualBox Version 3.1 (works for the beta as well) in the libvirt vbox driver. The patch for the same is attached below. Regards, Pritesh
On Tue, Nov 17, 2009 at 01:29:26PM +0100, pritesh wrote:
Hi All,
I have just added support for VirtualBox Version 3.1 (works for the beta as well) in the libvirt vbox driver.
The patch for the same is attached below.
To be honest this patch is far too large for me to review usefully, since there is a huge amount of refactoring of existing code. I'll just assume you've tested that it works & doesn't regress 2.2/3.0 version of VBox. So ACK once this release freeze is over Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2009/11/17 pritesh <Pritesh.Kothari@sun.com>:
Hi All,
I have just added support for VirtualBox Version 3.1 (works for the beta as well) in the libvirt vbox driver.
The patch for the same is attached below.
Regards, Pritesh
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.
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index cd60b72..ed4406f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c [...] @@ -351,6 +406,265 @@ static void vboxUtf8toIID(virConnectPtr conn, char *uuidstr, vboxIID **iid) { virReportOOMError(conn); }
+#if VBOX_API_VERSION >= 3001 + +/** + * function to generate the name for medium, + * for e.g: hda, sda, etc + * + * Limitation: 1) max (26+(26*26)+(26*26*26)) i.e + * 18,278 names for now + * + * @returns null terminated string with device name or NULL + * for failures + * @param conn Input Connection Pointer + * @param storageBus Input storage bus type + * @param deviceInst Input device instance number + * @param devicePort Input port number + * @param deviceSlot Input slot number + * @param aMaxPortPerInst Input array of max port per device instance + * @param aMaxSlotPerPort Input array of max slot per device port + * + */ +static char *vboxGenerateMediumName(virConnectPtr conn, + PRUint32 storageBus, + PRInt32 deviceInst, + PRInt32 devicePort, + PRInt32 deviceSlot, + PRUint32 *aMaxPortPerInst, + PRUint32 *aMaxSlotPerPort) { + char *name = NULL; + int len = 4; + int total = 0; + PRUint32 maxPortPerInst = 0; + PRUint32 maxSlotPerPort = 0; + + if ( !aMaxPortPerInst + || !aMaxSlotPerPort) + return NULL; + + maxPortPerInst = aMaxPortPerInst[storageBus]; + maxSlotPerPort = aMaxSlotPerPort[storageBus]; + total = (deviceInst * maxPortPerInst * maxSlotPerPort) + + (devicePort * maxSlotPerPort) + + deviceSlot; + + if ((total >= 26) && (total < 26*26 + 26)) + len = 5; + else if ((total >= 26*26 + 26) && (total < 26*26*26 + 26*26 + 26)) + len = 6; + + if (VIR_ALLOC_N(name, len) < 0) { + virReportOOMError(conn); + 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
Hi Matthias,
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.
Wow that was really great, thanks for the efforts you put in analyzing and reviewing the code. I really appreciate it. Also about not splitting the patch in two separate chunks, I tried but then some functions in 3.1 would break thus making it very dependent on second patch, and the second reason being, i need to sync the 3.1 code with internal svn (company policy ...) and generating patch from it is not so easy :(
[...]
+ 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.
thanks, handled this now.
+ 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.
handled this now as well
+ } 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].
that would be really appreciated, will use this function once available in 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.
yes will switch to 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?
oops, this is called sincere novice mistake ;) ,, will change this
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
once again thanks for review, will try to split the patch in two and repost it again which changes suggested above, but before that, would like to know if esxVMX_IndexToDiskName() goes into util.[ch] anytime soon or only after the release? Regards, Pritesh
Hi All,
ACK, apart from the minor issues in vboxGenerateMediumName() and vboxGetDeviceDetails().
Matthias
once again thanks for review, will try to split the patch in two and repost it again which changes suggested above, but before that, would like to know if esxVMX_IndexToDiskName() goes into util.[ch] anytime soon or only after the release?
Just fixed all the above mentioned bugs/quirks and also fixed serial port configuration which was broken due to recent change in virDomainChrDef where targetType was newly added. The patch applies on the current big hunk sent before. Regards, Pritesh
2009/11/20 pritesh <Pritesh.Kothari@sun.com>:
Hi All,
ACK, apart from the minor issues in vboxGenerateMediumName() and vboxGetDeviceDetails().
Matthias
once again thanks for review, will try to split the patch in two and repost it again which changes suggested above, but before that, would like to know if esxVMX_IndexToDiskName() goes into util.[ch] anytime soon or only after the release?
Just fixed all the above mentioned bugs/quirks and also fixed serial port configuration which was broken due to recent change in virDomainChrDef where targetType was newly added.
The patch applies on the current big hunk sent before.
Regards, Pritesh
I found a small issue with virDiskNameToIndex() while moving esxVMX_IndexToDiskName() to util.c that affects your vboxGenerateMediumName() too. virDiskNameToIndex() created adjacent indices up to 701. 701 maps to sdzz and sdaaa maps to 728. The indices in between have no disk name representation. Because of this your inverse mapping in vboxGenerateMediumName() will fail for indices > 701, but this can be solved once we have a general virIndexToDiskName() in util.c. I've posted patches to move esxVMX_IndexToDiskName() to util.c and to solve the gap problem or to handle it at least. ACK to your 2nd patch. Matthias
2009/11/20 Matthias Bolte <matthias.bolte@googlemail.com>:
2009/11/20 pritesh <Pritesh.Kothari@sun.com>:
Hi All,
ACK, apart from the minor issues in vboxGenerateMediumName() and vboxGetDeviceDetails().
Matthias
once again thanks for review, will try to split the patch in two and repost it again which changes suggested above, but before that, would like to know if esxVMX_IndexToDiskName() goes into util.[ch] anytime soon or only after the release?
Just fixed all the above mentioned bugs/quirks and also fixed serial port configuration which was broken due to recent change in virDomainChrDef where targetType was newly added.
The patch applies on the current big hunk sent before.
Regards, Pritesh
I found a small issue with virDiskNameToIndex() while moving esxVMX_IndexToDiskName() to util.c that affects your vboxGenerateMediumName() too. virDiskNameToIndex() created adjacent indices up to 701. 701 maps to sdzz and sdaaa maps to 728. The indices in between have no disk name representation. Because of this your inverse mapping in vboxGenerateMediumName() will fail for indices > 701, but this can be solved once we have a general virIndexToDiskName() in util.c.
I've posted patches to move esxVMX_IndexToDiskName() to util.c and to solve the gap problem or to handle it at least.
ACK to your 2nd patch.
Matthias
Okay, applied and pushed both patches. Matthias
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.
After a second look at the macros, they really do different stuff in < 3.1 and for 3.1 for e.g: the medium release has imedium object in the vtables for < 3.1 while for 3.1 and above it directly calls release and the imedium object is not there any more. #if VBOX_API_VERSION < 3001 #define VBOX_MEDIUM_RELEASE(arg) \ if(arg)\ (arg)->vtbl->imedium.nsisupports.Release((nsISupports *)(arg)) #else /* VBOX_API_VERSION >= 3001 */ #define VBOX_MEDIUM_RELEASE(arg) \ if(arg)\ (arg)->vtbl->nsisupports.Release((nsISupports *)(arg)) #endif /* VBOX_API_VERSION >= 3001 */ but still, will try to split the patch without breaking 3.1 Regards, Pritesh
2009/11/20 pritesh <Pritesh.Kothari@sun.com>:
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.
After a second look at the macros, they really do different stuff in < 3.1 and for 3.1 for e.g: the medium release has imedium object in the vtables for < 3.1 while for 3.1 and above it directly calls release and the imedium object is not there any more.
#if VBOX_API_VERSION < 3001
#define VBOX_MEDIUM_RELEASE(arg) \ if(arg)\ (arg)->vtbl->imedium.nsisupports.Release((nsISupports *)(arg))
#else /* VBOX_API_VERSION >= 3001 */
#define VBOX_MEDIUM_RELEASE(arg) \ if(arg)\ (arg)->vtbl->nsisupports.Release((nsISupports *)(arg))
#endif /* VBOX_API_VERSION >= 3001 */
but still, will try to split the patch without breaking 3.1
Regards, Pritesh
Well, don't waste your time trying to split it if it's not that simple. Matthias
participants (3)
-
Daniel P. Berrange -
Matthias Bolte -
pritesh