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

Re: [libvirt] [PATCH] esx: Add .vmdk storage volume creation



On 08/28/2010 01:53 PM, Matthias Bolte wrote:
+    /* Validate config */
+    tmp1 = strchr(def->name, '/');
+    tmp2 = strrchr(def->name, '/');
+
+    if (tmp1 == NULL || tmp1 == def->name ||
+        tmp2 == NULL || tmp2[1] == '\0') {

tmp2 cannot be NULL if tmp1 is not NULL, since if the string contains a / in a forward search, it will also contain one in a reverse search. Also, checking that tmp1 != def->name can be done with a single-byte probe. What about using a single temporary for a faster test:

tmp = strrchr(def->name, '/');
if (tmp == NULL || *def->name == '/' || tmp[1] == '\0') {

+        /*
+         * FIXME: The adapter type is a required parameter, but there is no
+         * way to let the user specifiy it in the volume XML config. Therefore,

s/specifiy/specify/

+         * default to 'busLogic' here.
+         */

Sounds like a good followup patch, but doesn't impact whether this one is okay as-is.

+        virtualDiskSpec->adapterType = (char *)"busLogic";
+
+        virtualDiskSpec->capacityKb->value = def->capacity / 1024; /* Scale from byte to kilobyte */

Do you need to round up? That is, should def->capacity of 1 result in 0 or 1024 bytes?

I think those points are minor enough to not need a v2, so:

ACK with those points addressed.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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