[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v10 07/17] qemu-io: Switch 'alloc' command to by
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v10 07/17] qemu-io: Switch 'alloc' command to byte-based length |
Date: |
Fri, 28 Apr 2017 22:52:26 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 |
On 28.04.2017 22:36, Eric Blake wrote:
> On 04/28/2017 03:09 PM, Max Reitz wrote:
>> On 28.04.2017 21:59, Eric Blake wrote:
>>> On 04/28/2017 02:46 PM, Max Reitz wrote:
>>>> On 27.04.2017 03:46, Eric Blake wrote:
>>>>> For the 'alloc' command, accepting an offset in bytes but a length
>>>>> in sectors, and reporting output in sectors, is confusing. Do
>>>>> everything in bytes, and adjust the expected output accordingly.
>>>>>
>>>>> Signed-off-by: Eric Blake <address@hidden>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>>>>>
>>>
>>>>> }
>>>>> + if (!QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE)) {
>>>>> + printf("bytes %" PRId64 " is not sector aligned\n",
>>>>
>>>> This isn't real English. :-)
>>>
>>> But, it's just copy-and-paste from the other instances you just reviewed
>>> in 6/17! [Translation - if I change this one, I also get to redo that one]
>>
>> No, those are "offset" and "count" -- all singular. "bytes" is plural. ;-)
>
> Then an obvious solution: s/bytes/count/ in the parameter name :)
>
> But I still get to redo those, to add the '-' in 'sector-aligned'.
Oh, right! Didn't even notice. Well, in real languages stuff like that
would have to be joined into a single word anyway.
>>> Which of these various alternatives (if any) looks better:
>>>
>>> bytes=511 is not sector-aligned
>>> 511 is not a sector-aligned value for 'bytes'
>>> requested 'bytes' of 511 is not sector-aligned
>>> alignment error: 511 bytes is not sector-aligned
>>> 'bytes' must be sector-aligned: 511
>>> your clever entry here...
>>
>> How about "byte count" instead of "bytes" or "bytes value", if really
>> want to have the exact spelling in there?
>>
>> For your entries above: (1) and (2) work for me (I like (2) a bit
>> better), (3) doesn't sound like real English either, and it should be
>> s/is/are/ in (4) (but it still sounds off with that change). (5) I
>> mostly dislike because I dislike error message of the form "This should
>> be X: $foo", I like "$foo is not X" better.
>
> Maybe this variation of (3) solves the singular/plural disconnect:
>
> request of 511 for 'bytes' is not sector-aligned
>
> which makes it obvious that the "request of 511" (singular) and not the
> parameter name (whether singular 'count' or plural 'bytes') is the
> subject. But it's a bit wordier than (2). So it looks like (2) may be
> a winner in all the situations. But I also think you convinced me to
> rename the command parameter; in my next spin, the help text will read:
>
> alloc offset [count] -- checks if offset is allocated in the file
>
> which starts to be non-trivial enough to drop R-b that you were willing
> to give for just an error message wording change.
Well, reviewing the change will be simple enough, so it doesn't really
matter to me. :-)
(I'm still a bit upset why you think that the average qemu-io user
cannot make the connection between "byte count" and a parameter named
"bytes". Because I am the average qemu-io user. Huff!)
Max
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v10 06/17] qemu-io: Don't open-code QEMU_IS_ALIGNED, (continued)
[Qemu-block] [PATCH v10 08/17] qemu-io: Switch 'map' output to byte-based reporting, Eric Blake, 2017/04/26
[Qemu-block] [PATCH v10 09/17] qcow2: Optimize write zero of unaligned tail cluster, Eric Blake, 2017/04/26
[Qemu-block] [PATCH v10 12/17] blkdebug: Sanity check block layer guarantees, Eric Blake, 2017/04/26
[Qemu-block] [PATCH v10 13/17] blkdebug: Refactor error injection, Eric Blake, 2017/04/26
[Qemu-block] [PATCH v10 15/17] blkdebug: Simplify override logic, Eric Blake, 2017/04/26
[Qemu-block] [PATCH v10 14/17] blkdebug: Add pass-through write_zero and discard support, Eric Blake, 2017/04/26