Re: [libvirt] [PATCH] esx: Add .vmdk storage volume creation
2010/8/30 Eric Blake <eblake(a)redhat.com>:
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') {
Yes, that's fine too. I'll fold that in.
> + /*
> + * 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 truncate here on purpose. The rest of the ESX driver does it this
way, also the rest of the libvirt codebase truncates in situations
like this. Therefore, I'll leave this as it is.
I think those points are minor enough to not need a v2, so:
ACK with those points addressed.
Thanks, pushed.
Matthias