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",
+ 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
--
2.9.3
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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
© 2016 - 2025 Red Hat, Inc.