Re: [Qemu-devel] [PATCH v4 3/6] qemu-io: Allow unaligned access by default

[Max Reitz]

On 05.05.2016 05:42, Eric Blake wrote:
> There's no reason to require the user to specify a flag just so
> they can pass in unaligned numbers.  Keep 'read -p' and 'write -p'
> as no-ops so that I don't have to hunt down and update all users
> of qemu-io, but otherwise make their behavior default as 'read' and
> 'write'.  Also fix 'write -z', 'readv', 'writev', 'writev',
> 'aio_read', 'aio_write', and 'aio_write -z'.  For now, 'read -b',
> 'multiwrite', 'write -b', and 'write -c' still require alignment.
> 
> qemu-iotest 23 is updated to match, as the only test that was
> previously explicitly expecting an error on an unaligned request.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  qemu-io-cmds.c             |   63 +-
>  tests/qemu-iotests/023.out | 2160 
> +++++++++++++++++++++++++++++---------------
>  2 files changed, 1452 insertions(+), 771 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index dc6b0dc..8bcf742 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -395,12 +395,6 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char 
> **argv, int nr_iov,
>              goto fail;
>          }
> 
> -        if (len & 0x1ff) {
> -            printf("length argument %" PRId64
> -                   " is not sector aligned\n", len);
> -            goto fail;
> -        }
> -
>          sizes[i] = len;
>          count += len;
>      }
> @@ -634,7 +628,7 @@ static void read_help(void)
>  " -b, -- read from the VM state rather than the virtual disk\n"
>  " -C, -- report statistics in a machine parsable format\n"
>  " -l, -- length for pattern verification (only with -P)\n"
> -" -p, -- allow unaligned access\n"
> +" -p, -- ignored for back-compat\n"

We're not so close to the 80 character limit here to justify shortening
this as much. ;-)

I really wouldn't mind a full "backwards compatibility".

>  " -P, -- use a pattern to verify read data\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
>  " -s, -- start offset for pattern verification (only with -P)\n"
> @@ -650,7 +644,7 @@ static const cmdinfo_t read_cmd = {
>      .cfunc      = read_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-abCpqv] [-P pattern [-s off] [-l len]] off len",
> +    .args       = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
>      .oneline    = "reads a number of bytes at a specified offset",
>      .help       = read_help,
>  };
> @@ -658,7 +652,7 @@ static const cmdinfo_t read_cmd = {
>  static int read_f(BlockBackend *blk, int argc, char **argv)
>  {
>      struct timeval t1, t2;
> -    bool Cflag = false, pflag = false, qflag = false, vflag = false;
> +    bool Cflag = false, qflag = false, vflag = false;
>      bool Pflag = false, sflag = false, lflag = false, bflag = false;
>      int c, cnt;
>      char *buf;
> @@ -686,7 +680,7 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>              }
>              break;
>          case 'p':
> -            pflag = true;
> +            /* Ignored for back-compat */
>              break;
>          case 'P':
>              Pflag = true;
> @@ -718,11 +712,6 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>          return qemuio_command_usage(&read_cmd);
>      }
> 
> -    if (bflag && pflag) {
> -        printf("-b and -p cannot be specified at the same time\n");
> -        return 0;
> -    }
> -
>      offset = cvtnum(argv[optind]);
>      if (offset < 0) {
>          print_cvtnum_err(offset, argv[optind]);
> @@ -753,7 +742,7 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>          return 0;
>      }
> 
> -    if (!pflag) {
> +    if (bflag) {
>          if (offset & 0x1ff) {
>              printf("offset %" PRId64 " is not sector aligned\n",
>                     offset);
> @@ -890,12 +879,6 @@ static int readv_f(BlockBackend *blk, int argc, char 
> **argv)
>      }
>      optind++;
> 
> -    if (offset & 0x1ff) {
> -        printf("offset %" PRId64 " is not sector aligned\n",
> -               offset);
> -        return 0;
> -    }
> -
>      nr_iov = argc - optind;
>      buf = create_iovec(blk, &qiov, &argv[optind], nr_iov, 0xab);
>      if (buf == NULL) {
> @@ -952,7 +935,7 @@ static void write_help(void)
>  " filled with a set pattern (0xcdcdcdcd).\n"
>  " -b, -- write to the VM state rather than the virtual disk\n"
>  " -c, -- write compressed data with blk_write_compressed\n"
> -" -p, -- allow unaligned access\n"
> +" -p, -- ignored for back-compat\n"
>  " -P, -- use different pattern to fill file\n"
>  " -C, -- report statistics in a machine parsable format\n"
>  " -q, -- quiet mode, do not show I/O statistics\n"
> @@ -968,7 +951,7 @@ static const cmdinfo_t write_cmd = {
>      .cfunc      = write_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-bcCpqz] [-P pattern ] off len",
> +    .args       = "[-bcCqz] [-P pattern ] off len",

Would you mind removing the space after "pattern" along with this change?

>      .oneline    = "writes a number of bytes at a specified offset",
>      .help       = write_help,
>  };
> @@ -976,7 +959,7 @@ static const cmdinfo_t write_cmd = {
>  static int write_f(BlockBackend *blk, int argc, char **argv)
>  {
>      struct timeval t1, t2;
> -    bool Cflag = false, pflag = false, qflag = false, bflag = false;
> +    bool Cflag = false, qflag = false, bflag = false;
>      bool Pflag = false, zflag = false, cflag = false;
>      int c, cnt;
>      char *buf = NULL;
> @@ -998,7 +981,7 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>              Cflag = true;
>              break;
>          case 'p':
> -            pflag = true;
> +            /* Ignored for back-compat */
>              break;
>          case 'P':
>              Pflag = true;
> @@ -1022,8 +1005,8 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>          return qemuio_command_usage(&write_cmd);
>      }
> 
> -    if (bflag + pflag + zflag > 1) {
> -        printf("-b, -p, or -z cannot be specified at the same time\n");
> +    if (bflag + zflag > 1) {

Could you make that "bflag && zflag" instead?

Adding booleans was fine for the meantime between patch 2 and this one,
but I'd rather not keep it in the long run.

Max

> +        printf("-b and -z cannot be specified at the same time\n");
>          return 0;
>      }
>

signature.asc
Description: OpenPGP digital signature