| Message ID | 20170427014626.11553-8-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <eblake@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > v10: rebase to code cleanup > v9: new patch > --- > qemu-io-cmds.c | 30 ++++++++++++++++++------------ > tests/qemu-iotests/019.out | 8 ++++---- > tests/qemu-iotests/common.pattern | 2 +- > 3 files changed, 23 insertions(+), 17 deletions(-) > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index fabc394..34f6707 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -1760,7 +1760,7 @@ out: > static int alloc_f(BlockBackend *blk, int argc, char **argv) > { > BlockDriverState *bs = blk_bs(blk); > - int64_t offset, sector_num, nb_sectors, remaining; > + int64_t offset, sector_num, nb_sectors, remaining, bytes; > char s1[64]; > int num, ret; > int64_t sum_alloc; > @@ -1776,18 +1776,24 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv) > } > > if (argc == 3) { > - nb_sectors = cvtnum(argv[2]); > - if (nb_sectors < 0) { > - print_cvtnum_err(nb_sectors, argv[2]); > + bytes = cvtnum(argv[2]); > + if (bytes < 0) { > + print_cvtnum_err(bytes, argv[2]); > return 0; > - } else if (nb_sectors > INT_MAX) { > - printf("length argument cannot exceed %d, given %s\n", > - INT_MAX, argv[2]); > + } else if (bytes > INT_MAX * BDRV_SECTOR_SIZE) { > + printf("length argument cannot exceed %llu, given %s\n", > + INT_MAX * BDRV_SECTOR_SIZE, argv[2]); > return 0; > } > } else { > - nb_sectors = 1; > + bytes = BDRV_SECTOR_SIZE; > } > + if (!QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE)) { > + printf("bytes %" PRId64 " is not sector aligned\n", This isn't real English. :-) With that fixed (somehow, you know better than me how to): Reviewed-by: Max Reitz <mreitz@redhat.com> > + bytes); > + return 0; > + } > + nb_sectors = bytes >> BDRV_SECTOR_BITS; > > remaining = nb_sectors; > sum_alloc = 0;
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 <eblake@redhat.com> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> } >> + 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] 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... > > With that fixed (somehow, you know better than me how to): Re-reading my various alternatives, I do think that /sector aligned/sector-aligned/ helps no matter what; and that the remaining trick is to use quoting or '=' or some other lexical trick to make it obvious that 'bytes' is a parameter name whose value 511 is invalid, rather than part of the actual error of a value that is not properly aligned. > > Reviewed-by: Max Reitz <mreitz@redhat.com> If you state a preference for one of my variants, then the respin will use that variant consistently and add your R-b.
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 <eblake@redhat.com> >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> > >>> } >>> + 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. ;-) > > 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. >> With that fixed (somehow, you know better than me how to): > > Re-reading my various alternatives, I do think that /sector > aligned/sector-aligned/ helps no matter what; and that the remaining > trick is to use quoting or '=' or some other lexical trick to make it > obvious that 'bytes' is a parameter name whose value 511 is invalid, > rather than part of the actual error of a value that is not properly > aligned. First of all, I think the user is clever enough to figure out that a description like "byte count" refers to the "bytes" parameter. ;-) Secondly, this is even more true because this is a debugging tool so we don't have to worry too much about... Let's say inexperienced users. Max
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 <eblake@redhat.com> >>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>> >> >>>> } >>>> + 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'. > >> >> 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.
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 <eblake@redhat.com> >>>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> >>> >>>>> } >>>>> + 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
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index fabc394..34f6707 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1760,7 +1760,7 @@ out: static int alloc_f(BlockBackend *blk, int argc, char **argv) { BlockDriverState *bs = blk_bs(blk); - int64_t offset, sector_num, nb_sectors, remaining; + int64_t offset, sector_num, nb_sectors, remaining, bytes; char s1[64]; int num, ret; int64_t sum_alloc; @@ -1776,18 +1776,24 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv) } if (argc == 3) { - nb_sectors = cvtnum(argv[2]); - if (nb_sectors < 0) { - print_cvtnum_err(nb_sectors, argv[2]); + bytes = cvtnum(argv[2]); + if (bytes < 0) { + print_cvtnum_err(bytes, argv[2]); return 0; - } else if (nb_sectors > INT_MAX) { - printf("length argument cannot exceed %d, given %s\n", - INT_MAX, argv[2]); + } else if (bytes > INT_MAX * BDRV_SECTOR_SIZE) { + printf("length argument cannot exceed %llu, given %s\n", + INT_MAX * BDRV_SECTOR_SIZE, argv[2]); return 0; } } else { - nb_sectors = 1; + bytes = BDRV_SECTOR_SIZE; } + if (!QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE)) { + printf("bytes %" PRId64 " is not sector aligned\n", + bytes); + return 0; + } + nb_sectors = bytes >> BDRV_SECTOR_BITS; remaining = nb_sectors; sum_alloc = 0; @@ -1811,8 +1817,8 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv) cvtstr(offset, s1, sizeof(s1)); - printf("%"PRId64"/%"PRId64" sectors allocated at offset %s\n", - sum_alloc, nb_sectors, s1); + printf("%"PRId64"/%"PRId64" bytes allocated at offset %s\n", + sum_alloc << BDRV_SECTOR_BITS, nb_sectors << BDRV_SECTOR_BITS, s1); return 0; } @@ -1822,8 +1828,8 @@ static const cmdinfo_t alloc_cmd = { .argmin = 1, .argmax = 2, .cfunc = alloc_f, - .args = "off [sectors]", - .oneline = "checks if a sector is present in the file", + .args = "offset [bytes]", + .oneline = "checks if offset is allocated in the file", }; diff --git a/tests/qemu-iotests/019.out b/tests/qemu-iotests/019.out index 0124264..17a7c03 100644 --- a/tests/qemu-iotests/019.out +++ b/tests/qemu-iotests/019.out @@ -542,8 +542,8 @@ Testing conversion with -B TEST_DIR/t.IMGFMT.base Checking if backing clusters are allocated when they shouldn't -0/128 sectors allocated at offset 1 MiB -0/128 sectors allocated at offset 4.001 GiB +0/65536 bytes allocated at offset 1 MiB +0/65536 bytes allocated at offset 4.001 GiB Reading === IO: pattern 42 @@ -1086,8 +1086,8 @@ Testing conversion with -o backing_file=TEST_DIR/t.IMGFMT.base Checking if backing clusters are allocated when they shouldn't -0/128 sectors allocated at offset 1 MiB -0/128 sectors allocated at offset 4.001 GiB +0/65536 bytes allocated at offset 1 MiB +0/65536 bytes allocated at offset 4.001 GiB Reading === IO: pattern 42 diff --git a/tests/qemu-iotests/common.pattern b/tests/qemu-iotests/common.pattern index ddfbca1..34f4a8d 100644 --- a/tests/qemu-iotests/common.pattern +++ b/tests/qemu-iotests/common.pattern @@ -18,7 +18,7 @@ function do_is_allocated() { local start=$1 - local size=$(( $2 / 512)) + local size=$2 local step=$3 local count=$4