[libvirt] [PATCH] esx: Add .vmdk storage volume creation
Eric Blake
eblake at redhat.com
Mon Aug 30 14:22:09 UTC 2010
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list