Re: [Qemu-devel] [PATCH v5 6/7] blkdebug: Add ability to override unmap geometries

[Eric Blake <eblake@redhat.com>]

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

On 02/15/2017 10:20 AM, Kevin Wolf wrote:
> Am 14.02.2017 um 20:25 hat Eric Blake geschrieben:
>> Make it easier to simulate various unusual hardware setups (for
>> example, recent commits 3482b9b and b8d0a98 affect the Dell
>> Equallogic iSCSI with its 15M preferred and maximum unmap and
>> write zero sizing, or b2f95fe deals with the Linux loopback
>> block device having a max_transfer of 64k), by allowing blkdebug
>> to wrap any other device with further restrictions on various
>> alignments.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>>  # @inject-error:    #optional array of error injection descriptions
>>  #
>>  # @set-state:       #optional array of state-change descriptions
>> @@ -2472,7 +2493,9 @@
>>  { 'struct': 'BlockdevOptionsBlkdebug',
>>    'data': { 'image': 'BlockdevRef',
>>              '*config': 'str',
>> -            '*align': 'int',
>> +            '*align': 'int', '*max-transfer': 'int32',
>> +            '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
>> +            '*opt-discard': 'int32', '*max-discard': 'int32',
> 
> Hm, strictly speaking, this schema allows for negative values. Should we
> document that they aren't allowed?

Doesn't power of two imply positive? But yes, I can tweak the wording.


>> -    /* Set request alignment */
>> +    /* Set alignment overrides */
>>      s->align = qemu_opt_get_size(opts, "align", 0);
>>      if (s->align && (s->align >= INT_MAX || !is_power_of_2(s->align))) {
>>          error_setg(errp, "Invalid alignment, align must be integer power of 2");
>>          goto fail_unref;
>>      }
>> +    align = MAX(s->align, bs->file->bs->bl.request_alignment);
>> +
>> +    s->max_transfer = qemu_opt_get_size(opts, "max-transfer", 0);
>> +    if (s->max_transfer &&
>> +        (s->max_transfer >= INT_MAX ||
>> +         !QEMU_IS_ALIGNED(s->max_transfer, align))) {
>> +        error_setg(errp, "Invalid max-transfer, must be multiple of align");
> 
> It's not that I'm generally a friend of unspecific messages, but in this
> case I think I would prefer being unspecific to the potentially wrong
> error message that is returned here. We're checking multiple conditions
> and the error message mentions only one of them as the reason, which may
> or may not be the right one.
> 
> This is the same for all new options.

I'm leaning towards "Cannot meet constraints with max-transfer %lld" as
being non-specific enough to cover all the cases while still describing
the problem to be fixed.  Any other wording suggestions would be
welcome, if someone has a particular bikeshed color they like.

-- 
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 --]