Re: [Qemu-devel] [PATCH v10] qemu-io: add pattern file for write command

[Eric Blake]

On 8/20/19 11:46 AM, Denis Plotnikov wrote:
> The patch allows to provide a pattern file for write
> command. There was no similar ability before.
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---

> @@ -983,8 +1057,9 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>      /* Some compilers get confused and warn if this is not initialized.  */
>      int64_t total = 0;
>      int pattern = 0xcd;
> +    const char *file_name = NULL;
>  
> -    while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
> +    while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {

This one looks odd (I would have preserved ordering by sticking s:
between q and u).  But a maintainer could fix that.

>          switch (c) {
>          case 'b':
>              bflag = true;
> @@ -1020,6 +1095,10 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>          case 'z':
>              zflag = true;
>              break;
> +        case 's':
> +            sflag = true;
> +            file_name = optarg;
> +            break;

Likewise, sorting the cases in the same order as the getopt() listing
helps in finding code during later edits.

> @@ -1088,7 +1168,14 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>      }
>  
>      if (!zflag) {
> -        buf = qemu_io_alloc(blk, count, pattern);
> +        if (sflag) {
> +            buf = qemu_io_alloc_from_file(blk, count, file_name);
> +            if (!buf) {
> +                return -EINVAL;
> +            }
> +        } else {
> +            buf = qemu_io_alloc(blk, count, pattern);
> +        }

Pre-existing, but it is odd that qemu_io_alloc() exit()s rather than
returning NULL on huge allocation requests that can't be met.  (Then
again, we have an early exit on any length > 2G, and 2G allocations tend
to succeed on modern development machines).  Perhaps it would be nice to
teach qemu-io to use blk_try_blockalign for more graceful handling even
on 32-bit platforms, but that's not the problem of your patch.

Option ordering is minor enough that I'm fine giving:

Reviewed-by: Eric Blake <address@hidden>

Now, to figure out which maintainer should take it.  Perhaps you want to
add a patch 2/1 that adds an iotest using this new mode, to a) ensure it
doesn't regress, and b) makes it reasonable to take in through the
iotest tree.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature