Re: [Qemu-devel] [PATCH v2 08/17] blkdebug: Set request_alignment during .bdrv_refresh_limits()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

On 06/22/2016 04:05 AM, Kevin Wolf wrote:

>>>>      /* Set request alignment */
>>>> -    align = qemu_opt_get_size(opts, "align", bs->request_alignment);
>>>> -    if (align > 0 && align < INT_MAX && !(align & (align - 1))) {
>>>> -        bs->request_alignment = align;
>>>> +    align = qemu_opt_get_size(opts, "align",
>>>> +                              bs->request_alignment ?: BDRV_SECTOR_SIZE);
>>>

>>> I think you should either just unconditionally use BDRV_SECTOR_SIZE or
>>> even better just store 0 in s->align if the option isn't given. Your
>>> implementation of blkdebug_refresh_limits() already leaves the default
>>> bs->request_alignment alone if s->align == 0.
>>
>> I like that idea; I guess that means I instead need to tweak the logic
>> to test if "align" is present in opts (in which case it must be
>> non-zero), or absent (in which case leaving things as 0 is a nicer
>> approach than trying to guess which default to stick things to).
> 
> Except that you don't have to check all of that explicitly, the default
> value for absent options is what the third parameter is for:
> 
>     align = qemu_opt_get_size(opts, "align", 0);

Previously, we explicitly reject the user passing in an explicit
'align':0.  If I go with this approach of 0 as the default, then I may
need to tweak docs.  But that sounds better, so I'll try it.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]