Re: [Qemu-devel] [PATCH v2 16/17] block: Split bdrv_merge_limits() from bdrv_refresh_limits()

[Eric Blake]

On 06/21/2016 08:12 AM, Kevin Wolf wrote:
> Am 14.06.2016 um 23:30 hat Eric Blake geschrieben:
>> The raw block driver was blindly copying all limits from bs->file,
>> even though: 1. the main bdrv_refresh_limits() already does this
>> for many of gthe limits, and 2. blindly copying from the children
>> can weaken any stricter limits that were already inherited from
>> the backing dhain during the main bdrv_refresh_limits().  Also,
>> the next patch is about to move .request_alignment into
>> BlockLimits, and that is a limit that should NOT be copied from
>> other layers in the BDS chain.
>>
>> Solve the issue by factoring out a new bdrv_merge_limits(),
>> and using that function to properly merge limits when comparing
>> two BlockDriverState objects.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

> 
> Or in fact...
> 
>> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
>> index b1d5237..379ce8d 100644
>> --- a/block/raw_bsd.c
>> +++ b/block/raw_bsd.c
>> @@ -152,7 +152,7 @@ static int raw_get_info(BlockDriverState *bs, 
>> BlockDriverInfo *bdi)
>>
>>  static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>  {
>> -    bs->bl = bs->file->bs->bl;
>> +    bdrv_merge_limits(&bs->bl, &bs->file->bs->bl);
>>  }

The blind copy was not completely redundant, but the argument that we
don't want a blind copy but instead want to rely on a merge_limits does
indeed mean that...

> 
> ...isn't this completely redundant because bdrv_refresh_limits() already
> called bdrv_merge_limits(&bs->bl, &bs->file->bs->bl)? We could simply
> remove the .bdrv_refresh_limits implementation from raw_bsd.

...this sounds like a better approach.  I'll try it for v3.

> 
> And then bdrv_merge_limits() could even be static (I think factoring it
> out is a good idea anyway because it removes some duplication).

Yes, even if static, it still gets called twice, and makes for a nicer
way to quickly see which direction limits are constrained in
(MIN_NON_ZERO vs. MAX), as well as to make any further tweaks in later
patches easier to do from one spot (for example, we may want to add
logic that clamps an opt limit [which uses MAX logic] to never exceed a
max limit [which uses MIN_NON_ZERO logic]).

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

signature.asc
Description: OpenPGP digital signature