Re: [Qemu-block] [PATCH v11 1/9] qemu-io: Improve alignment checks

[Max Reitz]

On 29.04.2017 21:14, Eric Blake wrote:
> Several copy-and-pasted alignment checks exist in qemu-io, which
> could use some minor improvements:
> 
> - Manual comparison against 0x1ff is not as clean as using our
> alignment macros (QEMU_IS_ALIGNED) from osdep.h.
> 
> - The error messages aren't quite grammatically correct.

Well, yes, s/sector aligned/sector-aligned/, but the rest was largely
correct, and I have to admit I liked them better how they were before --
if only because they were shorter. (Longer explanation: These are error
messages for an interface to be used by humans, and I as a human don't
like it very much if my programs are overly technical and cold to me. "X
is an invalid value for 'Y'" is very technical. "X is wrong for Y" or "X
Y does not work" sounds more familiar and nicer. I like my programs nice.)

But just a matter of taste, so:

Reviewed-by: Max Reitz <address@hidden>

> Suggested-by: Philippe Mathieu-Daudé <address@hidden>
> Suggested-by: Max Reitz <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v11: retitle [was "qemu-io: Don't open-code QEMU_IS_ALIGNED"], improve
> error messages
> v10: new patch
> ---
>  qemu-io-cmds.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 21af9e6..6a0024b 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -740,13 +740,13 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>      }
> 
>      if (bflag) {
> -        if (offset & 0x1ff) {
> -            printf("offset %" PRId64 " is not sector aligned\n",
> +        if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
> +            printf("%" PRId64 " is not a sector-aligned value for 
> 'offset'\n",
>                     offset);
>              return 0;
>          }
> -        if (count & 0x1ff) {
> -            printf("count %"PRId64" is not sector aligned\n",
> +        if (!QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE)) {
> +            printf("%"PRId64" is not a sector-aligned value for 'count'\n",
>                     count);
>              return 0;
>          }
> @@ -1050,14 +1050,14 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>      }
> 
>      if (bflag || cflag) {
> -        if (offset & 0x1ff) {
> -            printf("offset %" PRId64 " is not sector aligned\n",
> +        if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
> +            printf("%" PRId64 " is not a sector-aligned value for 
> 'offset'\n",
>                     offset);
>              return 0;
>          }
> 
> -        if (count & 0x1ff) {
> -            printf("count %"PRId64" is not sector aligned\n",
> +        if (!QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE)) {
> +            printf("%"PRId64" is not a sector-aligned value for 'count'\n",
>                     count);
>              return 0;
>          }
> @@ -1769,8 +1769,8 @@ static int alloc_f(BlockBackend *blk, int argc, char 
> **argv)
>      if (offset < 0) {
>          print_cvtnum_err(offset, argv[1]);
>          return 0;
> -    } else if (offset & 0x1ff) {
> -        printf("offset %" PRId64 " is not sector aligned\n",
> +    } else if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
> +        printf("%" PRId64 " is not a sector-aligned value for 'offset'\n",
>                 offset);
>          return 0;
>      }
>

signature.asc
Description: OpenPGP digital signature