[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