[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]