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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
