[PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Denis V. Lunev]

The problem this patch is trying to address is libguestfs behavior on the
appliance startup. It starts supporting to use root=UUID definition in
the kernel command line of its root filesystem using
    file --  /usr/lib64/guestfs/appliance/root
This works fine with RAW image, but we are using QCOW2 as a storage to
save a bit of file space and in this case we get
    QEMU QCOW Image (v3), 1610612736 bytes
instead of UUID of the root filesystem.

The solution is very simple - we should dump first 256k of the image file
like the follows
    qemu-io -c "read -V 0 256k" appliance | file -
which will provide correct result for all possible types of the appliance
storage.

Unfortunately, additional option for qemu-io is the only and the simplest
solution as '-v' creates very specific output, which requires to be
parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
much more intrusive.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Richard W.M. Jones <rjones@redhat.com>
---
P.S. Patch to libguestfs will follow.

 qemu-io-cmds.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index baeae86d8c..7aae9726cd 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -718,7 +718,7 @@ static const cmdinfo_t read_cmd = {
     .cfunc      = read_f,
     .argmin     = 2,
     .argmax     = -1,
-    .args       = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
+    .args       = "[-abCqvV] [-P pattern [-s off] [-l len]] off len",
     .oneline    = "reads a number of bytes at a specified offset",
     .help       = read_help,
 };
@@ -728,6 +728,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     struct timespec t1, t2;
     bool Cflag = false, qflag = false, vflag = false;
     bool Pflag = false, sflag = false, lflag = false, bflag = false;
+    bool vrawflag = true;
     int c, cnt, ret;
     char *buf;
     int64_t offset;
@@ -737,7 +738,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     int pattern = 0;
     int64_t pattern_offset = 0, pattern_count = 0;
 
-    while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
+    while ((c = getopt(argc, argv, "bCl:pP:qs:vV")) != -1) {
         switch (c) {
         case 'b':
             bflag = true;
@@ -777,6 +778,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
         case 'v':
             vflag = true;
             break;
+        case 'V':
+            vrawflag = true;
+            break;
         default:
             qemuio_command_usage(&read_cmd);
             return -EINVAL;
@@ -869,10 +873,15 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     if (vflag) {
         dump_buffer(buf, offset, count);
     }
+    if (vrawflag) {
+        write(STDOUT_FILENO, buf, count);
+    }
 
     /* Finally, report back -- -C gives a parsable format */
-    t2 = tsub(t2, t1);
-    print_report("read", &t2, offset, count, total, cnt, Cflag);
+    if (!vrawflag) {
+        t2 = tsub(t2, t1);
+        print_report("read", &t2, offset, count, total, cnt, Cflag);
+    }
 
 out:
     qemu_io_free(buf);
-- 
2.17.1

Re: [PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Richard W.M. Jones]

On Mon, Aug 10, 2020 at 03:35:55PM +0300, Denis V. Lunev wrote:
> The problem this patch is trying to address is libguestfs behavior on the
> appliance startup. It starts supporting to use root=UUID definition in
> the kernel command line of its root filesystem using
>     file --  /usr/lib64/guestfs/appliance/root
> This works fine with RAW image, but we are using QCOW2 as a storage to
> save a bit of file space and in this case we get
>     QEMU QCOW Image (v3), 1610612736 bytes
> instead of UUID of the root filesystem.
> 
> The solution is very simple - we should dump first 256k of the image file
> like the follows
>     qemu-io -c "read -V 0 256k" appliance | file -
> which will provide correct result for all possible types of the appliance
> storage.
> 
> Unfortunately, additional option for qemu-io is the only and the simplest
> solution as '-v' creates very specific output, which requires to be
> parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
> much more intrusive.

I like the idea of the flag - we could also use it in the nbdkit test
suite.

I wonder if the actual flag ('V') is a good idea because I would
normally associate that with getting the version of a command.  But
as these are subcommand flags, that's probably not too relevant here.

So I think this is fine from my point of view.

Rich.

> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Richard W.M. Jones <rjones@redhat.com>
> ---
> P.S. Patch to libguestfs will follow.
> 
>  qemu-io-cmds.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index baeae86d8c..7aae9726cd 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -718,7 +718,7 @@ static const cmdinfo_t read_cmd = {
>      .cfunc      = read_f,
>      .argmin     = 2,
>      .argmax     = -1,
> -    .args       = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
> +    .args       = "[-abCqvV] [-P pattern [-s off] [-l len]] off len",
>      .oneline    = "reads a number of bytes at a specified offset",
>      .help       = read_help,
>  };
> @@ -728,6 +728,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>      struct timespec t1, t2;
>      bool Cflag = false, qflag = false, vflag = false;
>      bool Pflag = false, sflag = false, lflag = false, bflag = false;
> +    bool vrawflag = true;
>      int c, cnt, ret;
>      char *buf;
>      int64_t offset;
> @@ -737,7 +738,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>      int pattern = 0;
>      int64_t pattern_offset = 0, pattern_count = 0;
>  
> -    while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
> +    while ((c = getopt(argc, argv, "bCl:pP:qs:vV")) != -1) {
>          switch (c) {
>          case 'b':
>              bflag = true;
> @@ -777,6 +778,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>          case 'v':
>              vflag = true;
>              break;
> +        case 'V':
> +            vrawflag = true;
> +            break;
>          default:
>              qemuio_command_usage(&read_cmd);
>              return -EINVAL;
> @@ -869,10 +873,15 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>      if (vflag) {
>          dump_buffer(buf, offset, count);
>      }
> +    if (vrawflag) {
> +        write(STDOUT_FILENO, buf, count);
> +    }
>  
>      /* Finally, report back -- -C gives a parsable format */
> -    t2 = tsub(t2, t1);
> -    print_report("read", &t2, offset, count, total, cnt, Cflag);
> +    if (!vrawflag) {
> +        t2 = tsub(t2, t1);
> +        print_report("read", &t2, offset, count, total, cnt, Cflag);
> +    }
>  
>  out:
>      qemu_io_free(buf);
> -- 
> 2.17.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Denis V. Lunev]

On 8/10/20 3:52 PM, Richard W.M. Jones wrote:
> On Mon, Aug 10, 2020 at 03:35:55PM +0300, Denis V. Lunev wrote:
>> The problem this patch is trying to address is libguestfs behavior on the
>> appliance startup. It starts supporting to use root=UUID definition in
>> the kernel command line of its root filesystem using
>>     file --  /usr/lib64/guestfs/appliance/root
>> This works fine with RAW image, but we are using QCOW2 as a storage to
>> save a bit of file space and in this case we get
>>     QEMU QCOW Image (v3), 1610612736 bytes
>> instead of UUID of the root filesystem.
>>
>> The solution is very simple - we should dump first 256k of the image file
>> like the follows
>>     qemu-io -c "read -V 0 256k" appliance | file -
>> which will provide correct result for all possible types of the appliance
>> storage.
>>
>> Unfortunately, additional option for qemu-io is the only and the simplest
>> solution as '-v' creates very specific output, which requires to be
>> parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
>> much more intrusive.
> I like the idea of the flag - we could also use it in the nbdkit test
> suite.
>
> I wonder if the actual flag ('V') is a good idea because I would
> normally associate that with getting the version of a command.  But
> as these are subcommand flags, that's probably not too relevant here.
we could use '-r' aka --raw and apply it only if -v present.

I think that some opinion from Kevin or Max would be helpful
here.

Den

Re: [PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Vladimir Sementsov-Ogievskiy]

10.08.2020 15:35, Denis V. Lunev wrote:
> The problem this patch is trying to address is libguestfs behavior on the
> appliance startup. It starts supporting to use root=UUID definition in
> the kernel command line of its root filesystem using
>      file --  /usr/lib64/guestfs/appliance/root
> This works fine with RAW image, but we are using QCOW2 as a storage to
> save a bit of file space and in this case we get
>      QEMU QCOW Image (v3), 1610612736 bytes
> instead of UUID of the root filesystem.
> 
> The solution is very simple - we should dump first 256k of the image file
> like the follows
>      qemu-io -c "read -V 0 256k" appliance | file -
> which will provide correct result for all possible types of the appliance
> storage.
> 
> Unfortunately, additional option for qemu-io is the only and the simplest
> solution as '-v' creates very specific output, which requires to be
> parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
> much more intrusive.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Richard W.M. Jones <rjones@redhat.com>
> ---
> P.S. Patch to libguestfs will follow.
> 
>   qemu-io-cmds.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index baeae86d8c..7aae9726cd 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -718,7 +718,7 @@ static const cmdinfo_t read_cmd = {
>       .cfunc      = read_f,
>       .argmin     = 2,
>       .argmax     = -1,
> -    .args       = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
> +    .args       = "[-abCqvV] [-P pattern [-s off] [-l len]] off len",
>       .oneline    = "reads a number of bytes at a specified offset",
>       .help       = read_help,
>   };
> @@ -728,6 +728,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>       struct timespec t1, t2;
>       bool Cflag = false, qflag = false, vflag = false;
>       bool Pflag = false, sflag = false, lflag = false, bflag = false;
> +    bool vrawflag = true;
>       int c, cnt, ret;
>       char *buf;
>       int64_t offset;
> @@ -737,7 +738,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>       int pattern = 0;
>       int64_t pattern_offset = 0, pattern_count = 0;
>   
> -    while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
> +    while ((c = getopt(argc, argv, "bCl:pP:qs:vV")) != -1) {
>           switch (c) {
>           case 'b':
>               bflag = true;
> @@ -777,6 +778,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>           case 'v':
>               vflag = true;
>               break;
> +        case 'V':
> +            vrawflag = true;
> +            break;
>           default:
>               qemuio_command_usage(&read_cmd);
>               return -EINVAL;
> @@ -869,10 +873,15 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>       if (vflag) {
>           dump_buffer(buf, offset, count);
>       }
> +    if (vrawflag) {
> +        write(STDOUT_FILENO, buf, count);
> +    }
>   
>       /* Finally, report back -- -C gives a parsable format */
> -    t2 = tsub(t2, t1);
> -    print_report("read", &t2, offset, count, total, cnt, Cflag);
> +    if (!vrawflag) {
> +        t2 = tsub(t2, t1);
> +        print_report("read", &t2, offset, count, total, cnt, Cflag);
> +    }
>   
>   out:
>       qemu_io_free(buf);
> 

I think -v and -V should be mutually exclusive, as combined output doesn't make real sense.
Still, I'm OK with it as is (as well as with -V renamed to -r). I can suggest also -d (aka dump).

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Eric Blake]

On 8/10/20 7:35 AM, Denis V. Lunev wrote:
> The problem this patch is trying to address is libguestfs behavior on the
> appliance startup. It starts supporting to use root=UUID definition in
> the kernel command line of its root filesystem using
>      file --  /usr/lib64/guestfs/appliance/root
> This works fine with RAW image, but we are using QCOW2 as a storage to
> save a bit of file space and in this case we get
>      QEMU QCOW Image (v3), 1610612736 bytes
> instead of UUID of the root filesystem.
> 
> The solution is very simple - we should dump first 256k of the image file
> like the follows
>      qemu-io -c "read -V 0 256k" appliance | file -
> which will provide correct result for all possible types of the appliance
> storage.

Is 'appliance' a qcow2 file?  If so, another way to accomplish this 
could include:

nbdkit streaming --run 'qemu-img convert --image-opts 
driver=raw,size=256,file.driver=qcow2,file.file.driver=file,file.file.filename=appliance 
"$uri"' | file -

which says to use qemu-img to open a length-limited view of the file, 
and stream the entire thing to an NBD server, where nbdkit then provides 
a way to convert that to a pipeline to feed into file (since qemu's NBD 
server doesn't directly like writing into a pipe).

I'm also wondering if the 'nbdcopy' program (not yet released in a 
stable version, but available in libnbd.git) could be put to use on this 
front, with some way to quickly combine it with qemu-nbd serving 
appliance.qcow2.

> 
> Unfortunately, additional option for qemu-io is the only and the simplest
> solution as '-v' creates very specific output, which requires to be
> parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
> much more intrusive.

'qemu-img dd' _should_ be merely syntactic sugar for 'qemu-img convert', 
but it isn't yet.  Until we rectify that feature parity (there are 
things that dd can't do like better output control and skipping, and 
there are things that convert can't do like length limiting and offset 
selection), hacking up 'qemu-io' (which is for debugging use only) is a 
tolerable short-term solution; but qemu-io was NEVER intended for stable 
use cases.  If adding this makes debugging qemu easier, then go for it; 
but if the real problem is that qemu-img is missing functionality, we 
should instead be focusing on fixing qemu-img.

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

Re: [PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Kevin Wolf]

Am 10.08.2020 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 10.08.2020 15:35, Denis V. Lunev wrote:
> > The problem this patch is trying to address is libguestfs behavior on the
> > appliance startup. It starts supporting to use root=UUID definition in
> > the kernel command line of its root filesystem using
> >      file --  /usr/lib64/guestfs/appliance/root
> > This works fine with RAW image, but we are using QCOW2 as a storage to
> > save a bit of file space and in this case we get
> >      QEMU QCOW Image (v3), 1610612736 bytes
> > instead of UUID of the root filesystem.
> > 
> > The solution is very simple - we should dump first 256k of the image file
> > like the follows
> >      qemu-io -c "read -V 0 256k" appliance | file -
> > which will provide correct result for all possible types of the appliance
> > storage.
> > 
> > Unfortunately, additional option for qemu-io is the only and the simplest
> > solution as '-v' creates very specific output, which requires to be
> > parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
> > much more intrusive.
> > 
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > CC: Kevin Wolf <kwolf@redhat.com>
> > CC: Max Reitz <mreitz@redhat.com>
> > CC: Richard W.M. Jones <rjones@redhat.com>
> > ---
> > P.S. Patch to libguestfs will follow.
> > 
> >   qemu-io-cmds.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> > index baeae86d8c..7aae9726cd 100644
> > --- a/qemu-io-cmds.c
> > +++ b/qemu-io-cmds.c
> > @@ -718,7 +718,7 @@ static const cmdinfo_t read_cmd = {
> >       .cfunc      = read_f,
> >       .argmin     = 2,
> >       .argmax     = -1,
> > -    .args       = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
> > +    .args       = "[-abCqvV] [-P pattern [-s off] [-l len]] off len",
> >       .oneline    = "reads a number of bytes at a specified offset",
> >       .help       = read_help,
> >   };
> > @@ -728,6 +728,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
> >       struct timespec t1, t2;
> >       bool Cflag = false, qflag = false, vflag = false;
> >       bool Pflag = false, sflag = false, lflag = false, bflag = false;
> > +    bool vrawflag = true;
> >       int c, cnt, ret;
> >       char *buf;
> >       int64_t offset;
> > @@ -737,7 +738,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
> >       int pattern = 0;
> >       int64_t pattern_offset = 0, pattern_count = 0;
> > -    while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
> > +    while ((c = getopt(argc, argv, "bCl:pP:qs:vV")) != -1) {
> >           switch (c) {
> >           case 'b':
> >               bflag = true;
> > @@ -777,6 +778,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
> >           case 'v':
> >               vflag = true;
> >               break;
> > +        case 'V':
> > +            vrawflag = true;
> > +            break;
> >           default:
> >               qemuio_command_usage(&read_cmd);
> >               return -EINVAL;
> > @@ -869,10 +873,15 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
> >       if (vflag) {
> >           dump_buffer(buf, offset, count);
> >       }
> > +    if (vrawflag) {
> > +        write(STDOUT_FILENO, buf, count);
> > +    }
> >       /* Finally, report back -- -C gives a parsable format */
> > -    t2 = tsub(t2, t1);
> > -    print_report("read", &t2, offset, count, total, cnt, Cflag);
> > +    if (!vrawflag) {
> > +        t2 = tsub(t2, t1);
> > +        print_report("read", &t2, offset, count, total, cnt, Cflag);
> > +    }
> >   out:
> >       qemu_io_free(buf);
> > 
> 
> I think -v and -V should be mutually exclusive, as combined output doesn't make real sense.
> Still, I'm OK with it as is (as well as with -V renamed to -r). I can suggest also -d (aka dump).

I like -d (maybe with an alias --dump), though in the end the naming is
secondary. I think having some flag like this is very useful.

How about adding the same thing to write, i.e. get the buffer to write
from stdin?

Kevin

Re: [PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Denis V. Lunev]

On 8/10/20 5:40 PM, Kevin Wolf wrote:
> Am 10.08.2020 um 16:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 10.08.2020 15:35, Denis V. Lunev wrote:
>>> The problem this patch is trying to address is libguestfs behavior on the
>>> appliance startup. It starts supporting to use root=UUID definition in
>>> the kernel command line of its root filesystem using
>>>      file --  /usr/lib64/guestfs/appliance/root
>>> This works fine with RAW image, but we are using QCOW2 as a storage to
>>> save a bit of file space and in this case we get
>>>      QEMU QCOW Image (v3), 1610612736 bytes
>>> instead of UUID of the root filesystem.
>>>
>>> The solution is very simple - we should dump first 256k of the image file
>>> like the follows
>>>      qemu-io -c "read -V 0 256k" appliance | file -
>>> which will provide correct result for all possible types of the appliance
>>> storage.
>>>
>>> Unfortunately, additional option for qemu-io is the only and the simplest
>>> solution as '-v' creates very specific output, which requires to be
>>> parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
>>> much more intrusive.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> CC: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Max Reitz <mreitz@redhat.com>
>>> CC: Richard W.M. Jones <rjones@redhat.com>
>>> ---
>>> P.S. Patch to libguestfs will follow.
>>>
>>>   qemu-io-cmds.c | 17 +++++++++++++----
>>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>>> index baeae86d8c..7aae9726cd 100644
>>> --- a/qemu-io-cmds.c
>>> +++ b/qemu-io-cmds.c
>>> @@ -718,7 +718,7 @@ static const cmdinfo_t read_cmd = {
>>>       .cfunc      = read_f,
>>>       .argmin     = 2,
>>>       .argmax     = -1,
>>> -    .args       = "[-abCqv] [-P pattern [-s off] [-l len]] off len",
>>> +    .args       = "[-abCqvV] [-P pattern [-s off] [-l len]] off len",
>>>       .oneline    = "reads a number of bytes at a specified offset",
>>>       .help       = read_help,
>>>   };
>>> @@ -728,6 +728,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>>>       struct timespec t1, t2;
>>>       bool Cflag = false, qflag = false, vflag = false;
>>>       bool Pflag = false, sflag = false, lflag = false, bflag = false;
>>> +    bool vrawflag = true;
>>>       int c, cnt, ret;
>>>       char *buf;
>>>       int64_t offset;
>>> @@ -737,7 +738,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>>>       int pattern = 0;
>>>       int64_t pattern_offset = 0, pattern_count = 0;
>>> -    while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
>>> +    while ((c = getopt(argc, argv, "bCl:pP:qs:vV")) != -1) {
>>>           switch (c) {
>>>           case 'b':
>>>               bflag = true;
>>> @@ -777,6 +778,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>>>           case 'v':
>>>               vflag = true;
>>>               break;
>>> +        case 'V':
>>> +            vrawflag = true;
>>> +            break;
>>>           default:
>>>               qemuio_command_usage(&read_cmd);
>>>               return -EINVAL;
>>> @@ -869,10 +873,15 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
>>>       if (vflag) {
>>>           dump_buffer(buf, offset, count);
>>>       }
>>> +    if (vrawflag) {
>>> +        write(STDOUT_FILENO, buf, count);
>>> +    }
>>>       /* Finally, report back -- -C gives a parsable format */
>>> -    t2 = tsub(t2, t1);
>>> -    print_report("read", &t2, offset, count, total, cnt, Cflag);
>>> +    if (!vrawflag) {
>>> +        t2 = tsub(t2, t1);
>>> +        print_report("read", &t2, offset, count, total, cnt, Cflag);
>>> +    }
>>>   out:
>>>       qemu_io_free(buf);
>>>
>> I think -v and -V should be mutually exclusive, as combined output doesn't make real sense.
>> Still, I'm OK with it as is (as well as with -V renamed to -r). I can suggest also -d (aka dump).
> I like -d (maybe with an alias --dump), though in the end the naming is
> secondary. I think having some flag like this is very useful.
>
> How about adding the same thing to write, i.e. get the buffer to write
> from stdin?

no prob, will do :)

Den

Re: [PATCH for 5.2 1/1] qemu-io: add -V flag for read sub-command

[Vladimir Sementsov-Ogievskiy]

10.08.2020 17:20, Eric Blake wrote:
> On 8/10/20 7:35 AM, Denis V. Lunev wrote:
>> The problem this patch is trying to address is libguestfs behavior on the
>> appliance startup. It starts supporting to use root=UUID definition in
>> the kernel command line of its root filesystem using
>>      file --  /usr/lib64/guestfs/appliance/root
>> This works fine with RAW image, but we are using QCOW2 as a storage to
>> save a bit of file space and in this case we get
>>      QEMU QCOW Image (v3), 1610612736 bytes
>> instead of UUID of the root filesystem.
>>
>> The solution is very simple - we should dump first 256k of the image file
>> like the follows
>>      qemu-io -c "read -V 0 256k" appliance | file -
>> which will provide correct result for all possible types of the appliance
>> storage.
> 
> Is 'appliance' a qcow2 file?  If so, another way to accomplish this could include:
> 
> nbdkit streaming --run 'qemu-img convert --image-opts driver=raw,size=256,file.driver=qcow2,file.file.driver=file,file.file.filename=appliance "$uri"' | file -

This adds a dependency on nbdkit streaming plugin, not installed by default in our RH-based distributive. Probably the simplest thing is just create a temporary file for qemu-img convert (or dd) target, and then call file command on it.

> 
> which says to use qemu-img to open a length-limited view of the file, and stream the entire thing to an NBD server, where nbdkit then provides a way to convert that to a pipeline to feed into file (since qemu's NBD server doesn't directly like writing into a pipe).
> 
> I'm also wondering if the 'nbdcopy' program (not yet released in a stable version, but available in libnbd.git) could be put to use on this front, with some way to quickly combine it with qemu-nbd serving appliance.qcow2.
> 
>>
>> Unfortunately, additional option for qemu-io is the only and the simplest
>> solution as '-v' creates very specific output, which requires to be
>> parsed. 'qemu-img dd of=/dev/stdout' does not work and the fix would be
>> much more intrusive.
> 
> 'qemu-img dd' _should_ be merely syntactic sugar for 'qemu-img convert', but it isn't yet.  Until we rectify that feature parity (there are things that dd can't do like better output control and skipping, and there are things that convert can't do like length limiting and offset selection), hacking up 'qemu-io' (which is for debugging use only) is a tolerable short-term solution; but qemu-io was NEVER intended for stable use cases.  If adding this makes debugging qemu easier, then go for it; but if the real problem is that qemu-img is missing functionality, we should instead be focusing on fixing qemu-img.
> 

Thanks for clarifying this!

If we are going to improve qemu-img at some point, I'm not sure that convert or dd is correct places to do it, as they want target to be a block node. But how to create a block-node on top of /dev/stdout? Detect it and behave another way in img_dd function? Or may be better create another command qemu-img dump, for dumping the contents to stdout?

-- 
Best regards,
Vladimir