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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list