Re: [PATCH v3 3/7] block: add max_hw_transfer to BlockLimits

[Paolo Bonzini <pbonzini@redhat.com>]

On 15/06/21 18:18, Max Reitz wrote:
>>   }
>> +/* Returns the maximum hardware transfer length, in bytes; guaranteed 
>> nonzero */
>> +uint64_t blk_get_max_hw_transfer(BlockBackend *blk)
>> +{
>> +    BlockDriverState *bs = blk_bs(blk);
>> +    uint64_t max = INT_MAX;
>> +
>> +    if (bs) {
>> +        max = MIN_NON_ZERO(bs->bl.max_hw_transfer, bs->bl.max_transfer);
>> +    }
>> +    return max;
> 
> Both `max_hw_transfer` and `max_transfer` can be 0, so this could return 
> 0, contrary to what the comment above promises.
> 
> Should `max` be initialized to 0 with a `MIN_NON_ZERO(max, INT_MAX)` 
> here (like `blk_get_max_transfer()` does it)?

Yes, something to that effect.

> (As for the rest, I think aligning to the request alignment makes sense, 
> but then again we don’t do that for max_transfer either, so... this at 
> least wouldn’t be a new bug.

Ok, will do.  I will also add a new patch to align max_transfer to the 
request alignment.

> Regarding the comment, checkpatch complains about it, so it should be 
> fixed so that /* is on its own line.

That makes it different from every other comment in block_int.h though. 
  Is it okay to fix all of them in a follow-up?

Paolo

> Speaking of checkpatch, now that I ran it, it also complains about the 
> new line in bdrv_merge_limits() exceeding 80 characters, so that should 
> be fixed, too.)