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

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



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


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