2009/11/17 pritesh <Pritesh.Kothari@xxxxxxx>:
> 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list