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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org