[Qemu-block] [PATCH] qemu-img-cmds.hx: Add example usage for create command

[John Arbuckle]

Add an example on how to use the create command. I believe this will make 
qemu-img easier to use.

Signed-off-by: John Arbuckle 
---
 qemu-img-cmds.hx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 69758fb6e8..92f7437944 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -50,7 +50,7 @@ STEXI
 ETEXI
 
 DEF("create", img_create,
-"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-u] [-o options] filename [size]")
+"create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-u] [-o options] filename [size]\nExample: qemu-img create -f 
qcow2 WindowsXP.qcow2 10G")
 STEXI
 @item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 
@var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] 
@var{filename} [@var{size}]
 ETEXI
-- 
2.14.3 (Apple Git-98)

Re: [Qemu-block] interactive qemu-img

[Programmingkid]

> On Jul 30, 2018, at 6:14 PM, Nir Soffer  wrote:
> 
> On Mon, Jul 30, 2018 at 11:09 PM Eric Blake  wrote:
> On 07/30/2018 02:54 PM, Nir Soffer wrote:
> 
> >>> $ qemu-img
> >>> qemu-img: Not enough arguments
> >>> Try 'qemu-img --help' for more information
> > 
> > 
> > This is not user friendly, but unfortunately very common.
> > It can be improved by treating no arguments as --help, like git.
> 
> I somewhat disagree that git is a good example.  If '--help' occupies 
> more than about 25 lines (a screenful on some default terminal sizes), 
> the mere fact that you have to scroll to read it makes it less helpful 
> than a 2-liner statement that lets you know "I couldn't do anything 
> useful with your botched command line, please read the documentation and 
> try again".  'git' behaving as 'git --help' outputs 42 lines, which 
> fails my 'one screenful' test; and 'qemu-img --help' at 104 lines is 
> definitely too verbose to be the default behavior when --help is not given.
> 
> /me Wow - I can't believe I'm actually about to use this as an example, but:
> 
> 'cvs' and 'cvs --help' is just 13 lines (except to stderr, when it 
> should have been stdout), and gives enough hints on how to get more 
> specific help on a particular topic.  Great for the plain 'cvs' case; a 
> bit more debatable on 'cvs --help' (the fact that you have to ask for 
> help twice: once for the summary, again for the specific help, gets 
> tedious).
> 
> At any rate, getting command-line tools to have user-friendly behavior 
> when insufficient arguments are supplied is an artform, and you'll find 
> lots of bikeshed colors out there.
> 
> And regardless of any opinions I've expressed above, this thread does 
> point out that 104 lines for 'qemu-img --help' is long, and anything we 
> can do to make that easier to digest may still be worthwhile.
> 
> I agree that git help is too long, and falling back to --help without 
> improving
> qemu-img help is not very useful.
> 
> What we need is something like:
> 
> $ qemu-img 
> 
> Commands:
>   amend   description...
>   bench   description...
>   check   description...
>   commit  description...
>   compare description...
>   convert description...
>   create  description...
>   dd  description...
>   infodescription...
>   map description...
>   measure description...
>   snapshotdescription...
>   rebase  description...
>   resize  description...
>   
> This fits in a terminal, and we have space for additional tips or common
> command line options.

This looks actually readable. I like the idea.

Re: [Qemu-block] interactive qemu-img

[Nir Soffer]

On Mon, Jul 30, 2018 at 11:09 PM Eric Blake  wrote:

> On 07/30/2018 02:54 PM, Nir Soffer wrote:
>
> >>> $ qemu-img
> >>> qemu-img: Not enough arguments
> >>> Try 'qemu-img --help' for more information
> >
> >
> > This is not user friendly, but unfortunately very common.
> > It can be improved by treating no arguments as --help, like git.
>
> I somewhat disagree that git is a good example.  If '--help' occupies
> more than about 25 lines (a screenful on some default terminal sizes),
> the mere fact that you have to scroll to read it makes it less helpful
> than a 2-liner statement that lets you know "I couldn't do anything
> useful with your botched command line, please read the documentation and
> try again".  'git' behaving as 'git --help' outputs 42 lines, which
> fails my 'one screenful' test; and 'qemu-img --help' at 104 lines is
> definitely too verbose to be the default behavior when --help is not given.
>
> /me Wow - I can't believe I'm actually about to use this as an example,
> but:
>
> 'cvs' and 'cvs --help' is just 13 lines (except to stderr, when it
> should have been stdout), and gives enough hints on how to get more
> specific help on a particular topic.  Great for the plain 'cvs' case; a
> bit more debatable on 'cvs --help' (the fact that you have to ask for
> help twice: once for the summary, again for the specific help, gets
> tedious).
>
> At any rate, getting command-line tools to have user-friendly behavior
> when insufficient arguments are supplied is an artform, and you'll find
> lots of bikeshed colors out there.
>
> And regardless of any opinions I've expressed above, this thread does
> point out that 104 lines for 'qemu-img --help' is long, and anything we
> can do to make that easier to digest may still be worthwhile.
>

I agree that git help is too long, and falling back to --help without
improving
qemu-img help is not very useful.

What we need is something like:

$ qemu-img

Commands:
  amend   description...
  bench   description...
  check   description...
  commit  description...
  compare description...
  convert description...
  create  description...
  dd  description...
  infodescription...
  map description...
  measure description...
  snapshotdescription...
  rebase  description...
  resize  description...

This fits in a terminal, and we have space for additional tips or common
command line options.

Note that this is not "botched command line", so we don't need to complain
about missing arguments.

qemu-img create --help

Should show only help for create.

virsh works in a similar way, but it has too many commands in the main help.

Of course this may break some management systems calling qemu-img --help
and parsing the output. We used to do this in oVirt in the past :-)

Nir

Re: [Qemu-block] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Jeff Cody]

On Sat, Jul 28, 2018 at 09:50:05AM +0200, Niels de Vos wrote:
> On Sat, Jul 28, 2018 at 12:18:39AM -0400, Jeff Cody wrote:
> > On Fri, Jul 27, 2018 at 08:24:05AM -0500, Eric Blake wrote:
> > > On 07/27/2018 03:19 AM, Niels de Vos wrote:
> > > >From: Prasanna Kumar Kalever 
> > > >
> > > >New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
> > > >function that returns additional 'struct stat' structures to enable
> > > >advanced caching of attributes. This is useful for file servers, not so
> > > >much for QEMU. Nevertheless, the API has changed and needs to be
> > > >adopted.
> > > >
> > > 
> > > Oh, one other comment.
> > > 
> > > >+++ b/block/gluster.c
> > > >@@ -20,6 +20,10 @@
> > > >  #include "qemu/option.h"
> > > >  #include "qemu/cutils.h"
> > > >+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> > > >+# define glfs_ftruncate(fd, offset, _u1, _u2) glfs_ftruncate(fd, offset)
> > > >+#endif
> > > 
> > > Someday, when we can assume new enough gluster everywhere, we can drop 
> > > this
> > > hunk...
> > > 
> > 
> > Part of me wishes that libgfapi had just created a new function
> > 'glfs_ftruncate2', so that existing users don't need to handle the api
> > change.  But I guess in the grand scheme, not a huge deal either way.
> 
> Gluster uses versioned symbols, so older binaries will keep working with
> new libraries. It is (hopefully) rare that existing symbols get updated.
> We try to send patches for these kind of changes to the projects we know
> well in advance, reducing the number of surprises.
> 
> > > >+++ b/configure
> > > 
> > > >+/* new glfs_ftruncate() passes two additional args */
> > > >+return glfs_ftruncate(NULL, 0 /*, NULL, NULL */);
> > > >+}
> > > >+EOF
> > > >+if compile_prog "$glusterfs_cflags" "$glusterfs_libs" ; then
> > > >+  glusterfs_legacy_ftruncate="yes"
> > > >+fi
> > > 
> > > ...but it will be easier to remember to do so if this comment in configure
> > > calls out the upstream gluster version that no longer requires the legacy
> > > workaround, as our hint for when...
> > > 
> > 
> > I can go ahead and add that to the comment in my branch after applying, if
> > Niels can let me know what that version is/will be (if known).
> 
> The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> October). We're changing the numbering scheme, it was expected to come
> in glusterfs-4.2, but that is a version that never will be released.
> 
> Thanks for correcting the last bits of the patch!
> Niels

So that there is no confusion or miscommunication: I'm not going to pull
this patch in through my tree for 3.0 (or 3.1 yet), since the new API isn't
part of a released version of gluster yet.  (And hopefully we won't ever
need it, if the gluster changes can happen without breaking the existing
API)

-Jeff

[Qemu-block] [PATCH] qemu-img.c: increase spacing between commands in documentation

[John Arbuckle]

When the user uses the --help option in qemu-img, the output for the commands 
is very hard to read due to being so close to each other. With this patch the 
help for the commands is double spaced making things easier to read.

Signed-off-by: John Arbuckle 
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 9b7506b8ae..6a7e63435e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -120,7 +120,7 @@ static void QEMU_NORETURN help(void)
"\n"
"Command syntax:\n"
 #define DEF(option, callback, arg_string)\
-   "  " arg_string "\n"
+   "  " arg_string "\n\n"
 #include "qemu-img-cmds.h"
 #undef DEF
"\n"
-- 
2.14.3 (Apple Git-98)

Re: [Qemu-block] interactive qemu-img

[Jeff Cody]

On Mon, Jul 30, 2018 at 04:57:54PM -0400, Programmingkid wrote:
> 
> > On Jul 30, 2018, at 3:55 PM, Jeff Cody  wrote:
> > 
> > On Mon, Jul 30, 2018 at 12:30:01PM -0400, Programmingkid wrote:
> >> 
> >>> On Jul 30, 2018, at 11:09 AM, Eric Blake  wrote:
> >>> 
> >>> On 07/28/2018 08:22 PM, Programmingkid wrote:
>  I thought of a way to make qemu-img much more user-friendly. When the 
>  user opens qemu-img without any arguments, we could present a prompt 
>  that guides the user on making an image file.
>  This illustrates what I think should happen.
>  
>  Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc, vvfat):
>  qcow2
>  Please enter a size (e.g. 100M, 10G):
>  4G
>  Please enter a name:
>  WinXP.qcow2
>  Creating image file...done
>  The interactive prompt would contain enough options to make a usable 
>  image file. If the user wants to use some of the more advanced features 
>  of qemu-img he or she would still need to use the command-line.
>  Would such a patch be welcomed?
> >>> 
> >>> qemu-img is a command line tool, not a gui.  Bloating it with a gui 
> >>> dialog box is probably not a wise idea.
> >> There would be no gui dialog box. Qemu-img would still be a command-line 
> >> tool. The patch would be done in printf/scanf calls.
> >> 
> > 
> > Even without a GUI, this would still add a not insignificant bloat and
> > unnecessary complexity to qemu-img, that doesn't add to the core
> > purpose of the tool.
> > 
> > It is not that the idea of such a dialog-driven tool (command-line or
> > otherwise) is without merit; it is that it would be better served as a
> > wrapper around qemu-img rather than built into qemu-img.  And it probably
> > wouldn't belong as part of the QEMU codebase, either, but more like other
> > management tools (e.g. libvirt) that wrap QEMU and add higher-level features
> > like that.
> > 
> > (One example of sorts, albeit of a GUI, is virt-manager. If you explore the
> > storage management, you can create qemu images of various types).
> 
> A wrapper around qemu-img might actually be a better idea than making 
> qemu-img interactive. I'm currently not sure which route to travel. Maybe 
> just improving the help of qemu-img might be good enough.
> 

Even as a standalone wrapper around qemu-img, there is still the question of
whether this is something to include in the QEMU git tree.

The biggest reason against it (IMO) is that it becomes another installed
tool that now has to be maintained from here on out (and not to mention,
essentially a management tool, at that).  I don't see why QEMU would start
shipping a management tool for qemu-img, when we don't ship any other
management tools that are user-facing.

I think an interactive wrapper could be nice, but it should be a separate
project outside of QEMU.  I also don't think this should discourage you from
making such a tool, if it is something you'd find useful.  Many a useful
project started from people creating tools for their own consumption!

-Jeff

Re: [Qemu-block] interactive qemu-img

[Programmingkid]

> On Jul 30, 2018, at 3:55 PM, Jeff Cody  wrote:
> 
> On Mon, Jul 30, 2018 at 12:30:01PM -0400, Programmingkid wrote:
>> 
>>> On Jul 30, 2018, at 11:09 AM, Eric Blake  wrote:
>>> 
>>> On 07/28/2018 08:22 PM, Programmingkid wrote:
 I thought of a way to make qemu-img much more user-friendly. When the user 
 opens qemu-img without any arguments, we could present a prompt that 
 guides the user on making an image file.
 This illustrates what I think should happen.
 
 Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc, vvfat):
 qcow2
 Please enter a size (e.g. 100M, 10G):
 4G
 Please enter a name:
 WinXP.qcow2
 Creating image file...done
 The interactive prompt would contain enough options to make a usable image 
 file. If the user wants to use some of the more advanced features of 
 qemu-img he or she would still need to use the command-line.
 Would such a patch be welcomed?
>>> 
>>> qemu-img is a command line tool, not a gui.  Bloating it with a gui dialog 
>>> box is probably not a wise idea.
>> There would be no gui dialog box. Qemu-img would still be a command-line 
>> tool. The patch would be done in printf/scanf calls.
>> 
> 
> Even without a GUI, this would still add a not insignificant bloat and
> unnecessary complexity to qemu-img, that doesn't add to the core
> purpose of the tool.
> 
> It is not that the idea of such a dialog-driven tool (command-line or
> otherwise) is without merit; it is that it would be better served as a
> wrapper around qemu-img rather than built into qemu-img.  And it probably
> wouldn't belong as part of the QEMU codebase, either, but more like other
> management tools (e.g. libvirt) that wrap QEMU and add higher-level features
> like that.
> 
> (One example of sorts, albeit of a GUI, is virt-manager. If you explore the
> storage management, you can create qemu images of various types).

A wrapper around qemu-img might actually be a better idea than making qemu-img 
interactive. I'm currently not sure which route to travel. Maybe just improving 
the help of qemu-img might be good enough.

> 
>>> Personally, I'm just fine with the current command line behavior:
>>> 
>>> $ qemu-img
>>> qemu-img: Not enough arguments
>>> Try 'qemu-img --help' for more information
>>> 
>>> as 'qemu-img --help' tells you how to properly use the command, without 
>>> having to hand-hold you through the process.
>> Hand holding feels way better than the coldness of the --help option.

Re: [Qemu-block] interactive qemu-img

[Programmingkid]

> On Jul 30, 2018, at 3:54 PM, Nir Soffer  wrote:
> 
> On Mon, Jul 30, 2018 at 8:00 PM Programmingkid  
> wrote:
> 
> > On Jul 30, 2018, at 11:09 AM, Eric Blake  wrote:
> > 
> > On 07/28/2018 08:22 PM, Programmingkid wrote:
> >> I thought of a way to make qemu-img much more user-friendly. When the user 
> >> opens qemu-img without any arguments, we could present a prompt that 
> >> guides the user on making an image file.
> >> This illustrates what I think should happen.
> >> 
> >> Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc, vvfat):
> >> qcow2
> >> Please enter a size (e.g. 100M, 10G):
> >> 4G
> >> Please enter a name:
> >> WinXP.qcow2
> >> Creating image file...done
> >> The interactive prompt would contain enough options to make a usable image 
> >> file. If the user wants to use some of the more advanced features of 
> >> qemu-img he or she would still need to use the command-line.
> >> Would such a patch be welcomed?
> > 
> > qemu-img is a command line tool, not a gui.  Bloating it with a gui dialog 
> > box is probably not a wise idea.
> There would be no gui dialog box. Qemu-img would still be a command-line 
> tool. The patch would be done in printf/scanf calls.
> 
> > Personally, I'm just fine with the current command line behavior:
> > 
> > $ qemu-img
> > qemu-img: Not enough arguments
> > Try 'qemu-img --help' for more information
> 
> This is not user friendly, but unfortunately very common.
> It can be improved by treating no arguments as --help, like git.

This would save the user a step.

> > 
> > as 'qemu-img --help' tells you how to properly use the command, without 
> > having to hand-hold you through the process.
> Hand holding feels way better than the coldness of the --help option.
> 
> User feeling that --help is too cold will be better served by management 
> system
> like virt-manager or oVirt. Did you try one of these?

I haven't but it is definitely an option.

> 
> Nir

Thank you.

Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command

[Programmingkid]

> On Jul 30, 2018, at 3:48 PM, Eric Blake  wrote:
> 
> On 07/30/2018 02:14 PM, John Arbuckle wrote:
>> Changes qemu-img so if the user runs it without any
>> arguments, it will walk the user thru making an image
>> file.
> 
> Please remember to cc qemu-devel on ALL patches, as suggested by 
> ./scripts/getmaintainer.pl.

Sorry about that. I did remember a few minutes
after sending the patch and sent it.

> 
>> Signed-off-by: John Arbuckle 
>> ---
>>  qemu-img.c | 31 +--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> Missing documentation patches (at a minimum, 'man qemu-img' should be updated 
> to mention this new mode of operation).  I'm also going to be a stickler and 
> require an addition to tests/qemu-iotests to ensure this new feature gets 
> coverage and doesn't regress (at least, if others are even in favor of the 
> idea. Personally, I'm 60-40 against the bloat, as telling the user to read 
> --help is sufficient in my mind).

After talking to Max Reitz about this issue I'm
not even sure I should continue with this plan.

Another idea I have is to improve the
documentation to qemu-img. Right now it looks
very hard to look at. I'm thinking a few examples
might make things easier for the user.

> 
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 9b7506b8ae..aa3df3b431 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -4873,6 +4873,32 @@ out:
>>  return ret;
>>  }
>>  +/* Guides the user on making an image file */
>> +static int interactive_mode()
>> +{
>> +char format[100];
>> +char size[100];
>> +char name[1000];
> 
> Eww. Fixed size buffers for user-provided input are evil.  getline() is your 
> friend.
> 
>> +
>> +printf("\nInteractive mode (Enter Control-C to cancel)\n");
>> +printf("Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, 
>> vpc): ");
> 
> Incomplete.  Better to generate the list dynamically by iterating over the 
> QAPI enum than it is to hard-code an incomplete list - particularly since 
> distros can white- or black-list particular drivers.
> 
>> +scanf("%100s", format);
> 
> Eww. Repeating the limit of your fixed size buffer, without checking that you 
> actually read a complete line, or that the scanf actually succeeded in 
> reading.  That's dangerous if a later programmer changes one but not both of 
> the two lines that are supposed to be using the same fixed size.  Even if we 
> accept the direction that this patch is heading in, it would need to be done 
> using more robust code.
> 
>> +printf("Please enter a size (e.g. 100M, 10G): ");
>> +scanf("%100s", size);
>> +printf("Please enter a name: ");
>> +scanf("%1000s", name);
> 
> A limit of 1000 has no bearing on actual file name lengths; a more typical 
> limit of NAME_MAX and/or PATH_MAX may come into play first (as low as 256 on 
> many systems, but on some systems, that value is intentionally undefined 
> because there is no inherent limit).  Again, you should be using getline() 
> and malloc'ing your result rather than using arbitrary (and probably 
> wrong-size) fixed-size buffers.
> 
>> +
>> +const char *arguments[] = {"create", "-f", format, name, size};
> 
> Although this is valid C99, qemu tends to prefer that you declare all 
> variables at the start of the scope rather than after statements.
> 
> You did not do ANY error checking that you actually parsed arguments from the 
> user; that in turn could result in passing bogus arguments on to img_create() 
> that could not normally happen via command line usage.

I was going to do error handling until I realized
img_create() is very good at telling the user
what is wrong. Anything I would add would be
redundant to what img_create() does.

> 
> Why are you hard-coding a create, rather than going FULLY interactive and 
> asking the user which sub-command (create, compare, ...) that they want to 
> use?

I was only thinking about what I use qemu-img for.
You do have a good point. A fully interactive
qemu-img command would be better.

>  Note that if we do want the level of interactivity to encompass the choice 
> of subcommand, that you still need to make things programmatic, not 
> hard-coded.  Also, John Snow had started some work on making qemu-img.c and 
> associated documentation saner, including making subcommands more uniform; 
> you may want to rebase your work on top of his cleanups, if there is even 
> demand for this.
> 
>> +int arg_count = 5;
>> +int return_value;
>> +return_value = img_create(arg_count, (char **)arguments);
> 
> Can we come up with a way to not have to cast away the const, if we are 
> locally supplying the arguments?
> 
>> +if (return_value == 0) {
>> +printf("Done creating image file\n");
>> +}
>> +
>> +return return_value;
>> +}
>> +
>>  static const img_cmd_t img_cmds[] = {
>>  #define DEF(option, callback, arg_string)\
>>  { option, callback },
>> @@ -4912,8 +4938,9 @@ int main(int argc, cha

Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command

[Programmingkid]

> On Jul 30, 2018, at 3:39 PM, Max Reitz  wrote:
> 
> On 2018-07-30 21:14, John Arbuckle wrote:
>> Changes qemu-img so if the user runs it without any
>> arguments, it will walk the user thru making an image
>> file.
>> 
>> Signed-off-by: John Arbuckle 
>> ---
>> qemu-img.c | 31 +--
>> 1 file changed, 29 insertions(+), 2 deletions(-)
> 
> I'm not fully opposed to this, but I would prefer quite a different
> implementation.
> 
> So first, there are technical issues with this patch, let me start (at
> least the ones I can think of right now):
> 
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 9b7506b8ae..aa3df3b431 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -4873,6 +4873,32 @@ out:
>> return ret;
>> }
>> 
>> +/* Guides the user on making an image file */
>> +static int interactive_mode()
>> +{
>> +char format[100];
>> +char size[100];
>> +char name[1000];
> 
> First, we'd want to check isatty(), because interactive mode doesn't
> make sense without.

Sounds good.

> 
>> +
>> +printf("\nInteractive mode (Enter Control-C to cancel)\n");
>> +printf("Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, 
>> vpc): ");
> 
> This list is not complete.
> 
>> +scanf("%100s", format);
> 
> Your first buffer overflow is right here because the field width doesn't
> include the terminating null byte.

Ok. I will change the values to 99, 99, and 999.

> 
>> +printf("Please enter a size (e.g. 100M, 10G): ");
>> +scanf("%100s", size);
>> +printf("Please enter a name: ");
>> +scanf("%1000s", name);
> 
> I'd say 1000 characters are rather arbitrary.  Also, I personally really
> like to have tab completion any time I have to specify a filename.  Hm,
> well, qemu-io doesn't have that either in interactive mode, but then
> again qemu-io is just a debugging tool so it doesn't have to live up to
> any standards.

That was the thing that bugged me the most. I looked up what was the max
path length in Linux, Mac OS X, and Windows. Then I realized that all 
three operating systems don't just have one file system but multiple. 
Each file system has its own limitations. My original approach was to 
check the host operating system like this:

#ifdef Linux
...
#elif defined(__MACH__)
...
#endif

This would not work because it doesn't look at the file system in use on
that platform. Other than creating a very extensive database of file
systems and maximum file paths, I think an arbitrary value might be ok
for now. Not a perfect solution, but something that should work most of
the time.

> 
>> +
>> +const char *arguments[] = {"create", "-f", format, name, size};
>> +int arg_count = 5;
>> +int return_value;
> 
> Variable declarations must be located at the start of the block.

I always liked keeping related code together, but this shouldn't be a
problem.

> 
>> +return_value = img_create(arg_count, (char **)arguments);
>> +if (return_value == 0) {
>> +printf("Done creating image file\n");
> 
> I think the output by img_create() is usually sufficiently verbose.

Seeing that text did look better to me, but I can do without it.

> 
>> +}
>> +
>> +return return_value;
>> +}
>> +
> 
> So that's nothing that couldn't be fixed.  But I have a design issue,
> and it's the following: If we want such a feature, it should work for
> all commands and it should work automatically, ideally.  Now parsing
> qemu-img-cmds.hx is probably over the top, I don't know.  But I do know
> that I don't quite like this ad-hoc interface in this patch that only
> works for create, and even doesn't really work.

I thought the patch made qemu-img very usable. I only use it to create
image files so my opinion is very biased. The other commands that are
available are ones I never use but shouldn't be too hard to implement
interactively. It would probably mean adding a new output statement:

printf("What would you like to do: (amend, bench, check, commit, compare, 
create, convert, dd, info, map, measure, snapshot, rebase, resize): ");

> What comes to my mind is this: Why don't you write a front-end for
> qemu-img?  It'd be trivial to write a script that performs the
> interactive mode, offers nice features such as tab completion (because
> you can write it in a scripting language, which makes such things easy),
> and then feeds the result to qemu-img.

Interesting idea. A cross platform solution could be made using something
like Tkinter. 

> The drawback of course is the following: It wouldn't be in qemu-img.  So
> you'd need "advertising" for it.  Putting it in the qemu source tree
> would be a bit out of the question, because it'd basically be a
> management tool, so it should be separate.  And when we're talking about
> management tools...  Well, there are already management tools that allow
> you image creation with bells and whistles, so, well.

I got the idea of the interactive mode from bochs. It has a very extensive
set of options that it w

Re: [Qemu-block] interactive qemu-img

[Eric Blake]

On 07/30/2018 02:54 PM, Nir Soffer wrote:


$ qemu-img
qemu-img: Not enough arguments
Try 'qemu-img --help' for more information



This is not user friendly, but unfortunately very common.
It can be improved by treating no arguments as --help, like git.


I somewhat disagree that git is a good example.  If '--help' occupies 
more than about 25 lines (a screenful on some default terminal sizes), 
the mere fact that you have to scroll to read it makes it less helpful 
than a 2-liner statement that lets you know "I couldn't do anything 
useful with your botched command line, please read the documentation and 
try again".  'git' behaving as 'git --help' outputs 42 lines, which 
fails my 'one screenful' test; and 'qemu-img --help' at 104 lines is 
definitely too verbose to be the default behavior when --help is not given.


/me Wow - I can't believe I'm actually about to use this as an example, but:

'cvs' and 'cvs --help' is just 13 lines (except to stderr, when it 
should have been stdout), and gives enough hints on how to get more 
specific help on a particular topic.  Great for the plain 'cvs' case; a 
bit more debatable on 'cvs --help' (the fact that you have to ask for 
help twice: once for the summary, again for the specific help, gets 
tedious).


At any rate, getting command-line tools to have user-friendly behavior 
when insufficient arguments are supplied is an artform, and you'll find 
lots of bikeshed colors out there.


And regardless of any opinions I've expressed above, this thread does 
point out that 104 lines for 'qemu-img --help' is long, and anything we 
can do to make that easier to digest may still be worthwhile.


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

Re: [Qemu-block] interactive qemu-img

[Jeff Cody]

On Mon, Jul 30, 2018 at 12:30:01PM -0400, Programmingkid wrote:
> 
> > On Jul 30, 2018, at 11:09 AM, Eric Blake  wrote:
> > 
> > On 07/28/2018 08:22 PM, Programmingkid wrote:
> >> I thought of a way to make qemu-img much more user-friendly. When the user 
> >> opens qemu-img without any arguments, we could present a prompt that 
> >> guides the user on making an image file.
> >> This illustrates what I think should happen.
> >> 
> >> Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc, vvfat):
> >> qcow2
> >> Please enter a size (e.g. 100M, 10G):
> >> 4G
> >> Please enter a name:
> >> WinXP.qcow2
> >> Creating image file...done
> >> The interactive prompt would contain enough options to make a usable image 
> >> file. If the user wants to use some of the more advanced features of 
> >> qemu-img he or she would still need to use the command-line.
> >> Would such a patch be welcomed?
> > 
> > qemu-img is a command line tool, not a gui.  Bloating it with a gui dialog 
> > box is probably not a wise idea.
> There would be no gui dialog box. Qemu-img would still be a command-line 
> tool. The patch would be done in printf/scanf calls.
> 

Even without a GUI, this would still add a not insignificant bloat and
unnecessary complexity to qemu-img, that doesn't add to the core
purpose of the tool.

It is not that the idea of such a dialog-driven tool (command-line or
otherwise) is without merit; it is that it would be better served as a
wrapper around qemu-img rather than built into qemu-img.  And it probably
wouldn't belong as part of the QEMU codebase, either, but more like other
management tools (e.g. libvirt) that wrap QEMU and add higher-level features
like that.

(One example of sorts, albeit of a GUI, is virt-manager. If you explore the
storage management, you can create qemu images of various types).

> > Personally, I'm just fine with the current command line behavior:
> > 
> > $ qemu-img
> > qemu-img: Not enough arguments
> > Try 'qemu-img --help' for more information
> > 
> > as 'qemu-img --help' tells you how to properly use the command, without 
> > having to hand-hold you through the process.
> Hand holding feels way better than the coldness of the --help option.

Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command

[Eric Blake]

On 07/30/2018 02:14 PM, John Arbuckle wrote:

Changes qemu-img so if the user runs it without any
arguments, it will walk the user thru making an image
file.


Please remember to cc qemu-devel on ALL patches, as suggested by 
./scripts/getmaintainer.pl.




Signed-off-by: John Arbuckle 
---
  qemu-img.c | 31 +--
  1 file changed, 29 insertions(+), 2 deletions(-)



Missing documentation patches (at a minimum, 'man qemu-img' should be 
updated to mention this new mode of operation).  I'm also going to be a 
stickler and require an addition to tests/qemu-iotests to ensure this 
new feature gets coverage and doesn't regress (at least, if others are 
even in favor of the idea. Personally, I'm 60-40 against the bloat, as 
telling the user to read --help is sufficient in my mind).



diff --git a/qemu-img.c b/qemu-img.c
index 9b7506b8ae..aa3df3b431 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4873,6 +4873,32 @@ out:
  return ret;
  }
  
+/* Guides the user on making an image file */

+static int interactive_mode()
+{
+char format[100];
+char size[100];
+char name[1000];


Eww. Fixed size buffers for user-provided input are evil.  getline() is 
your friend.



+
+printf("\nInteractive mode (Enter Control-C to cancel)\n");
+printf("Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc): 
");


Incomplete.  Better to generate the list dynamically by iterating over 
the QAPI enum than it is to hard-code an incomplete list - particularly 
since distros can white- or black-list particular drivers.



+scanf("%100s", format);


Eww. Repeating the limit of your fixed size buffer, without checking 
that you actually read a complete line, or that the scanf actually 
succeeded in reading.  That's dangerous if a later programmer changes 
one but not both of the two lines that are supposed to be using the same 
fixed size.  Even if we accept the direction that this patch is heading 
in, it would need to be done using more robust code.



+printf("Please enter a size (e.g. 100M, 10G): ");
+scanf("%100s", size);
+printf("Please enter a name: ");
+scanf("%1000s", name);


A limit of 1000 has no bearing on actual file name lengths; a more 
typical limit of NAME_MAX and/or PATH_MAX may come into play first (as 
low as 256 on many systems, but on some systems, that value is 
intentionally undefined because there is no inherent limit).  Again, you 
should be using getline() and malloc'ing your result rather than using 
arbitrary (and probably wrong-size) fixed-size buffers.



+
+const char *arguments[] = {"create", "-f", format, name, size};


Although this is valid C99, qemu tends to prefer that you declare all 
variables at the start of the scope rather than after statements.


You did not do ANY error checking that you actually parsed arguments 
from the user; that in turn could result in passing bogus arguments on 
to img_create() that could not normally happen via command line usage.


Why are you hard-coding a create, rather than going FULLY interactive 
and asking the user which sub-command (create, compare, ...) that they 
want to use?  Note that if we do want the level of interactivity to 
encompass the choice of subcommand, that you still need to make things 
programmatic, not hard-coded.  Also, John Snow had started some work on 
making qemu-img.c and associated documentation saner, including making 
subcommands more uniform; you may want to rebase your work on top of his 
cleanups, if there is even demand for this.



+int arg_count = 5;
+int return_value;
+return_value = img_create(arg_count, (char **)arguments);


Can we come up with a way to not have to cast away the const, if we are 
locally supplying the arguments?



+if (return_value == 0) {
+printf("Done creating image file\n");
+}
+
+return return_value;
+}
+
  static const img_cmd_t img_cmds[] = {
  #define DEF(option, callback, arg_string)\
  { option, callback },
@@ -4912,8 +4938,9 @@ int main(int argc, char **argv)
  
  module_call_init(MODULE_INIT_QOM);

  bdrv_init();
-if (argc < 2) {
-error_exit("Not enough arguments");
+
+if (argc == 1) { /* If no arguments passed to qemu-img */
+return interactive_mode();


This is placed early-enough in main() that you never use interactive 
mode if the user passed options but no subcommand (such as 'qemu-img -T 
foo'), is that intentional?  Conversely, your patch does not help if a 
user calls 'qemu-img create' without options, while it can be argued 
that if we want (partial) interactivity, why not be nice to the user and 
provide it at any step of the way rather than just when no subcommand 
name was given.


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

Re: [Qemu-block] interactive qemu-img

[Nir Soffer]

On Mon, Jul 30, 2018 at 8:00 PM Programmingkid 
wrote:

>
> > On Jul 30, 2018, at 11:09 AM, Eric Blake  wrote:
> >
> > On 07/28/2018 08:22 PM, Programmingkid wrote:
> >> I thought of a way to make qemu-img much more user-friendly. When the
> user opens qemu-img without any arguments, we could present a prompt that
> guides the user on making an image file.
> >> This illustrates what I think should happen.
> >> 
> >> Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc, vvfat):
> >> qcow2
> >> Please enter a size (e.g. 100M, 10G):
> >> 4G
> >> Please enter a name:
> >> WinXP.qcow2
> >> Creating image file...done
> >> The interactive prompt would contain enough options to make a usable
> image file. If the user wants to use some of the more advanced features of
> qemu-img he or she would still need to use the command-line.
> >> Would such a patch be welcomed?
> >
> > qemu-img is a command line tool, not a gui.  Bloating it with a gui
> dialog box is probably not a wise idea.
> There would be no gui dialog box. Qemu-img would still be a command-line
> tool. The patch would be done in printf/scanf calls.
>
> > Personally, I'm just fine with the current command line behavior:
> >
> > $ qemu-img
> > qemu-img: Not enough arguments
> > Try 'qemu-img --help' for more information


This is not user friendly, but unfortunately very common.
It can be improved by treating no arguments as --help, like git.


> >
> > as 'qemu-img --help' tells you how to properly use the command, without
> having to hand-hold you through the process.
> Hand holding feels way better than the coldness of the --help option.
>

User feeling that --help is too cold will be better served by management
system
like virt-manager or oVirt. Did you try one of these?

Nir

Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command

[Max Reitz]

On 2018-07-30 21:14, John Arbuckle wrote:
> Changes qemu-img so if the user runs it without any
> arguments, it will walk the user thru making an image
> file.
> 
> Signed-off-by: John Arbuckle 
> ---
>  qemu-img.c | 31 +--
>  1 file changed, 29 insertions(+), 2 deletions(-)

I'm not fully opposed to this, but I would prefer quite a different
implementation.

So first, there are technical issues with this patch, let me start (at
least the ones I can think of right now):

> diff --git a/qemu-img.c b/qemu-img.c
> index 9b7506b8ae..aa3df3b431 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4873,6 +4873,32 @@ out:
>  return ret;
>  }
>  
> +/* Guides the user on making an image file */
> +static int interactive_mode()
> +{
> +char format[100];
> +char size[100];
> +char name[1000];

First, we'd want to check isatty(), because interactive mode doesn't
make sense without.

> +
> +printf("\nInteractive mode (Enter Control-C to cancel)\n");
> +printf("Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc): 
> ");

This list is not complete.

> +scanf("%100s", format);

Your first buffer overflow is right here because the field width doesn't
include the terminating null byte.

> +printf("Please enter a size (e.g. 100M, 10G): ");
> +scanf("%100s", size);
> +printf("Please enter a name: ");
> +scanf("%1000s", name);

I'd say 1000 characters are rather arbitrary.  Also, I personally really
like to have tab completion any time I have to specify a filename.  Hm,
well, qemu-io doesn't have that either in interactive mode, but then
again qemu-io is just a debugging tool so it doesn't have to live up to
any standards.

> +
> +const char *arguments[] = {"create", "-f", format, name, size};
> +int arg_count = 5;
> +int return_value;

Variable declarations must be located at the start of the block.

> +return_value = img_create(arg_count, (char **)arguments);
> +if (return_value == 0) {
> +printf("Done creating image file\n");

I think the output by img_create() is usually sufficiently verbose.

> +}
> +
> +return return_value;
> +}
> +

So that's nothing that couldn't be fixed.  But I have a design issue,
and it's the following: If we want such a feature, it should work for
all commands and it should work automatically, ideally.  Now parsing
qemu-img-cmds.hx is probably over the top, I don't know.  But I do know
that I don't quite like this ad-hoc interface in this patch that only
works for create, and even doesn't really work.

What comes to my mind is this: Why don't you write a front-end for
qemu-img?  It'd be trivial to write a script that performs the
interactive mode, offers nice features such as tab completion (because
you can write it in a scripting language, which makes such things easy),
and then feeds the result to qemu-img.

The drawback of course is the following: It wouldn't be in qemu-img.  So
you'd need "advertising" for it.  Putting it in the qemu source tree
would be a bit out of the question, because it'd basically be a
management tool, so it should be separate.  And when we're talking about
management tools...  Well, there are already management tools that allow
you image creation with bells and whistles, so, well.


But the thing is that I don't oppose an interactive mode in qemu-img in
principle, because I believe there are indeed things that we'd need it
for.  But those are probably different from what you imagine.  I think
we'd need it in two cases:

(1) For asking the user when a potentially dangerous decision has to be
made.  For instance, we require --shrink for shrinking images because
that may lead to data loss.  Calling qemu-img twice just so the user can
confirm with --shrink is a bit stupid.  An interactive mode would be
nicer so we could just ask "Shrinking an image may result in data loss,
are you sure? [y/n] ".

(2) Commit is currently completely broken because it relies on the user
to specify filenames, and that is just not working reliably.  (The user
has to guess a filename and qemu may disagree that this is the correct
one.)  I believe we need an interactive mode here so we can present the
backing chain to the user and ask what layer should be committed where.


However, I don't quite see the point of putting an interactive mode for
the plain command-line interface into qemu-img, when you can achieve
exactly the same by putting a wrapper around it.  On the contrary, a
wrapper would allow you nicer functionality because you wouldn't have to
write it in C.

Max

>  static const img_cmd_t img_cmds[] = {
>  #define DEF(option, callback, arg_string)\
>  { option, callback },
> @@ -4912,8 +4938,9 @@ int main(int argc, char **argv)
>  
>  module_call_init(MODULE_INIT_QOM);
>  bdrv_init();
> -if (argc < 2) {
> -error_exit("Not enough arguments");
> +
> +if (argc == 1) { /* If no arguments passed to qemu-img */
> +return interactive_m

Re: [Qemu-block] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Jeff Cody]

On Mon, Jul 30, 2018 at 10:07:27AM -0500, Eric Blake wrote:
> On 07/28/2018 02:50 AM, Niels de Vos wrote:
> >>
> >>Part of me wishes that libgfapi had just created a new function
> >>'glfs_ftruncate2', so that existing users don't need to handle the api
> >>change.  But I guess in the grand scheme, not a huge deal either way.
> >
> >Gluster uses versioned symbols, so older binaries will keep working with
> >new libraries. It is (hopefully) rare that existing symbols get updated.
> >We try to send patches for these kind of changes to the projects we know
> >well in advance, reducing the number of surprises.
> 
> >>I can go ahead and add that to the comment in my branch after applying, if
> >>Niels can let me know what that version is/will be (if known).
> >
> >The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> >October). We're changing the numbering scheme, it was expected to come
> >in glusterfs-4.2, but that is a version that never will be released.
> >
> 
> Wait - so you're saying gluster has not yet released the incompatible
> change? Now would be the right time to get rid of the API breakage, before
> you bake it in, rather than relying solely on the versioned symbols to avoid
> an ABI breakage but forcing all clients to compensate to the API breakage.
> 

If this is not yet in a released version of Gluster, I'm not real eager to
pollute the QEMU driver codebase with #ifdef's, especially if there is a
possibility the API change may not actually materialize.

Is there any reason that this change is being approached in a way that
breaks API usage, and is it too late in the Gluster development pipeline to
change that?

[Qemu-block] [PATCH] Add interactive mode to qemu-img command

[John Arbuckle]

Changes qemu-img so if the user runs it without any
arguments, it will walk the user thru making an image
file.

Signed-off-by: John Arbuckle 
---
 qemu-img.c | 31 +--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 9b7506b8ae..aa3df3b431 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4873,6 +4873,32 @@ out:
 return ret;
 }
 
+/* Guides the user on making an image file */
+static int interactive_mode()
+{
+char format[100];
+char size[100];
+char name[1000];
+
+printf("\nInteractive mode (Enter Control-C to cancel)\n");
+printf("Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc): 
");
+scanf("%100s", format);
+printf("Please enter a size (e.g. 100M, 10G): ");
+scanf("%100s", size);
+printf("Please enter a name: ");
+scanf("%1000s", name);
+
+const char *arguments[] = {"create", "-f", format, name, size};
+int arg_count = 5;
+int return_value;
+return_value = img_create(arg_count, (char **)arguments);
+if (return_value == 0) {
+printf("Done creating image file\n");
+}
+
+return return_value;
+}
+
 static const img_cmd_t img_cmds[] = {
 #define DEF(option, callback, arg_string)\
 { option, callback },
@@ -4912,8 +4938,9 @@ int main(int argc, char **argv)
 
 module_call_init(MODULE_INIT_QOM);
 bdrv_init();
-if (argc < 2) {
-error_exit("Not enough arguments");
+
+if (argc == 1) { /* If no arguments passed to qemu-img */
+return interactive_mode();
 }
 
 qemu_add_opts(&qemu_object_opts);
-- 
2.14.3 (Apple Git-98)

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.0 0/7] fix persistent bitmaps migration logic

[John Snow]

On 07/30/2018 01:06 PM, Michael Roth wrote:
> Quoting John Snow (2018-07-23 17:22:03)
>> This is an updated version of Vladimir's proposal for fixing the
>> handling around migration and persistent dirty bitmaps.
> 
> Are these still being considered for 3.0 rc3/rc4? 2.12.1 releases this week
> and I'm not sure how badly these are needed.
> 

Good question. I'd still LIKE them in 3.0, but further fixes are
actually still needed, and I'm working on this right now.

I think you'll find that you might have trouble applying them to 2.12.1
without a fairly long trail of prerequisites, so it might not be easily
plausible.

>>
>> Patches 1, 4, 6, and 7 update the testing for this feature.
>> Patch 2 touches up an error message.
>> Patch 3 removes dead code.
>> Patch 5 contains the real fix.
>>
>> v2:
>>  - Add a new patch 4 as a prerequisite for what's now patch 5
>>  - Rework the fix to be (hopefully) cleaner, see patch 5 notes
>>  - Adjust error message in patch 2 (Eric)
>>  - Adjust test logic slightly (patches 6, 7) to deal with changes
>>in patch 5.
>>
>> John Snow (2):
>>   iotests: 169: actually test block migration
>>   dirty-bitmaps: clean-up bitmaps loading and migration logic
>>
>> Vladimir Sementsov-Ogievskiy (5):
>>   iotests: 169: drop deprecated 'autoload' parameter
>>   block/qcow2: improve error message in qcow2_inactivate
>>   block/qcow2: drop dirty_bitmaps_loaded state variable
>>   iotests: improve 169
>>   iotests: 169: add cases for source vm resuming
>>
>>  block.c|  4 ---
>>  block/dirty-bitmap.c   | 20 
>>  block/qcow2-bitmap.c   | 16 +
>>  block/qcow2.c  | 26 ---
>>  block/qcow2.h  |  1 -
>>  include/block/dirty-bitmap.h   |  2 +-
>>  migration/block-dirty-bitmap.c | 11 ---
>>  tests/qemu-iotests/169 | 74 
>> --
>>  tests/qemu-iotests/169.out |  4 +--
>>  9 files changed, 103 insertions(+), 55 deletions(-)
>>
>> -- 
>> 2.14.4
>>
>>

Re: [Qemu-block] [Qemu-devel] [PATCH for-3.0 0/7] fix persistent bitmaps migration logic

[Michael Roth]

Quoting John Snow (2018-07-23 17:22:03)
> This is an updated version of Vladimir's proposal for fixing the
> handling around migration and persistent dirty bitmaps.

Are these still being considered for 3.0 rc3/rc4? 2.12.1 releases this week
and I'm not sure how badly these are needed.

> 
> Patches 1, 4, 6, and 7 update the testing for this feature.
> Patch 2 touches up an error message.
> Patch 3 removes dead code.
> Patch 5 contains the real fix.
> 
> v2:
>  - Add a new patch 4 as a prerequisite for what's now patch 5
>  - Rework the fix to be (hopefully) cleaner, see patch 5 notes
>  - Adjust error message in patch 2 (Eric)
>  - Adjust test logic slightly (patches 6, 7) to deal with changes
>in patch 5.
> 
> John Snow (2):
>   iotests: 169: actually test block migration
>   dirty-bitmaps: clean-up bitmaps loading and migration logic
> 
> Vladimir Sementsov-Ogievskiy (5):
>   iotests: 169: drop deprecated 'autoload' parameter
>   block/qcow2: improve error message in qcow2_inactivate
>   block/qcow2: drop dirty_bitmaps_loaded state variable
>   iotests: improve 169
>   iotests: 169: add cases for source vm resuming
> 
>  block.c|  4 ---
>  block/dirty-bitmap.c   | 20 
>  block/qcow2-bitmap.c   | 16 +
>  block/qcow2.c  | 26 ---
>  block/qcow2.h  |  1 -
>  include/block/dirty-bitmap.h   |  2 +-
>  migration/block-dirty-bitmap.c | 11 ---
>  tests/qemu-iotests/169 | 74 
> --
>  tests/qemu-iotests/169.out |  4 +--
>  9 files changed, 103 insertions(+), 55 deletions(-)
> 
> -- 
> 2.14.4
> 
> 

Re: [Qemu-block] interactive qemu-img

[Programmingkid]

> On Jul 30, 2018, at 11:09 AM, Eric Blake  wrote:
> 
> On 07/28/2018 08:22 PM, Programmingkid wrote:
>> I thought of a way to make qemu-img much more user-friendly. When the user 
>> opens qemu-img without any arguments, we could present a prompt that guides 
>> the user on making an image file.
>> This illustrates what I think should happen.
>> 
>> Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc, vvfat):
>> qcow2
>> Please enter a size (e.g. 100M, 10G):
>> 4G
>> Please enter a name:
>> WinXP.qcow2
>> Creating image file...done
>> The interactive prompt would contain enough options to make a usable image 
>> file. If the user wants to use some of the more advanced features of 
>> qemu-img he or she would still need to use the command-line.
>> Would such a patch be welcomed?
> 
> qemu-img is a command line tool, not a gui.  Bloating it with a gui dialog 
> box is probably not a wise idea.
There would be no gui dialog box. Qemu-img would still be a command-line tool. 
The patch would be done in printf/scanf calls.

> Personally, I'm just fine with the current command line behavior:
> 
> $ qemu-img
> qemu-img: Not enough arguments
> Try 'qemu-img --help' for more information
> 
> as 'qemu-img --help' tells you how to properly use the command, without 
> having to hand-hold you through the process.
Hand holding feels way better than the coldness of the --help option.

Re: [Qemu-block] [PATCH] virtio-blk: fix comment for virtio_blk_rw_complete

[Yaowei Bai]

Oh, sorry, this patch should go into trivial mail list.

On Sat, Jul 28, 2018 at 01:18:44PM +0800, Yaowei Bai wrote:
> Here should be submit_requests, there is no submit_merged_requests
> function.
> 
> Signed-off-by: Yaowei Bai 
> ---
>  hw/block/virtio-blk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 50b5c86..91cbede 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -97,8 +97,8 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>  
>  if (req->qiov.nalloc != -1) {
>  /* If nalloc is != 1 req->qiov is a local copy of the original
> - * external iovec. It was allocated in submit_merged_requests
> - * to be able to merge requests. */
> + * external iovec. It was allocated in submit_requests to be
> + * able to merge requests. */
>  qemu_iovec_destroy(&req->qiov);
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [Qemu-block] [PATCH 1/6 for-3.0] Update .gitignore

[Eric Blake]

On 07/30/2018 10:40 AM, Eric Blake wrote:

On 07/29/2018 04:27 PM, Leonid Bloch wrote:

Adds /roms/vgabios to .gitignore - this directory appears after the
build, and was not ignored by Git so far.



Actually, that's the directories for one of the submodules. It seems 
fishy to have a directory listed in both .gitignore and .gitmodules at 
the same time, so I'm inclined to NACK this patch, and instead figure 
out what in the build is dirtying the directory when the submodule is 
not checked out.


Or rather, it was a submodule until commit 91b8eba9. So maybe what's 
happening is that you previously had it checked out, and now that we no 
longer have the submodule, it shows up unless .gitconfig ignores it. For 
that, I'd recommend editing .git/info/exclude to ignore it locally (if 
you plan on frequently checking out points both before and after the 
deletion) rather than making it part of upstream (which tends to focus 
more on ignoring files built by the current contents of master, not 
files that are leftovers from earlier commits).


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

Re: [Qemu-block] [PATCH 1/6 for-3.0] Update .gitignore

[Eric Blake]

On 07/29/2018 04:27 PM, Leonid Bloch wrote:

Adds /roms/vgabios to .gitignore - this directory appears after the
build, and was not ignored by Git so far.



Actually, that's the directories for one of the submodules. It seems 
fishy to have a directory listed in both .gitignore and .gitmodules at 
the same time, so I'm inclined to NACK this patch, and instead figure 
out what in the build is dirtying the directory when the submodule is 
not checked out.



Signed-off-by: Leonid Bloch 
---
  .gitignore | 1 +
  1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 5668d02782..2b3b30ae9f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -212,3 +212,4 @@ trace-dtrace-root.dtrace
  trace-ust-all.h
  trace-ust-all.c
  /target/arm/decode-sve.inc.c
+/roms/vgabios



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

[Qemu-block] [PULL 11/13] block/qapi: Add 'qdev' field to query-blockstats result

[Kevin Wolf]

Like for query-block, the client needs to identify which BlockBackend
the returned data is for. Anonymous BlockBackends are identified by the
device model they are attached to. Add a 'qdev' field that contains the
qdev ID or QOM path of the attached device model.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 qapi/block-core.json | 14 ++
 block/qapi.c | 10 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index d40d5ecc3b..5b9084a394 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -866,6 +866,9 @@
 #
 # @node-name: The node name of the device. (Since 2.3)
 #
+# @qdev: The qdev ID, or if no ID is assigned, the QOM path of the block
+#device. (since 3.0)
+#
 # @stats:  A @BlockDeviceStats for the device.
 #
 # @parent: This describes the file block device if it has one.
@@ -879,7 +882,7 @@
 # Since: 0.14.0
 ##
 { 'struct': 'BlockStats',
-  'data': {'*device': 'str', '*node-name': 'str',
+  'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
'stats': 'BlockDeviceStats',
'*parent': 'BlockStats',
'*backing': 'BlockStats'} }
@@ -941,7 +944,8 @@
 #"idle_time_ns":2953431879,
 #"account_invalid":true,
 #"account_failed":false
-# }
+# },
+# "qdev": "/machine/unattached/device[23]"
 #  },
 #  {
 # "device":"ide1-cd0",
@@ -959,7 +963,8 @@
 #"wr_merged":0,
 #"account_invalid":false,
 #"account_failed":false
-# }
+# },
+# "qdev": "/machine/unattached/device[24]"
 #  },
 #  {
 # "device":"floppy0",
@@ -977,7 +982,8 @@
 #"wr_merged":0,
 #"account_invalid":false,
 #"account_failed":false
-# }
+# },
+# "qdev": "/machine/unattached/device[16]"
 #  },
 #  {
 # "device":"sd0",
diff --git a/block/qapi.c b/block/qapi.c
index e12968fec8..50f867d634 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -597,11 +597,21 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
 BlockStatsList *info = g_malloc0(sizeof(*info));
 AioContext *ctx = blk_get_aio_context(blk);
 BlockStats *s;
+char *qdev;
 
 aio_context_acquire(ctx);
 s = bdrv_query_bds_stats(blk_bs(blk), true);
 s->has_device = true;
 s->device = g_strdup(blk_name(blk));
+
+qdev = blk_get_attached_dev_id(blk);
+if (qdev && *qdev) {
+s->has_qdev = true;
+s->qdev = qdev;
+} else {
+g_free(qdev);
+}
+
 bdrv_query_blk_stats(s->stats, blk);
 aio_context_release(ctx);
 
-- 
2.13.6

[Qemu-block] [PULL 10/13] file-posix: Fix write_zeroes with unmap on block devices

[Kevin Wolf]

The BLKDISCARD ioctl doesn't guarantee that the discarded blocks read as
all-zero afterwards, so don't try to abuse it for zero writing. We try
to only use this if BLKDISCARDZEROES tells us that it is safe, but this
is unreliable on older kernels and a constant 0 in newer kernels. In
other words, this code path is never actually used with newer kernels,
so we don't even try to unmap while writing zeros.

This patch removes the abuse of discard for writing zeroes from
file-posix and instead adds a new function that uses interfaces that are
actually meant to deallocate and zero out at the same time. Only if
those fail, it falls back to zeroing out without unmap. We never fall
back to a discard operation any more that may or may not result in
zeros.

Signed-off-by: Kevin Wolf 
---
 block/file-posix.c | 59 --
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 928b863ced..fe83cbf0eb 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -648,7 +648,7 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 }
 #endif
 
-bs->supported_zero_flags = s->discard_zeroes ? BDRV_REQ_MAY_UNMAP : 0;
+bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
 ret = 0;
 fail:
 if (filename && (bdrv_flags & BDRV_O_TEMPORARY)) {
@@ -1487,6 +1487,35 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 return -ENOTSUP;
 }
 
+static ssize_t handle_aiocb_write_zeroes_unmap(RawPosixAIOData *aiocb)
+{
+BDRVRawState *s G_GNUC_UNUSED = aiocb->bs->opaque;
+int ret;
+
+/* First try to write zeros and unmap at the same time */
+
+#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
+ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+   aiocb->aio_offset, aiocb->aio_nbytes);
+if (ret != -ENOTSUP) {
+return ret;
+}
+#endif
+
+#ifdef CONFIG_XFS
+if (s->is_xfs) {
+/* xfs_discard() guarantees that the discarded area reads as all-zero
+ * afterwards, so we can use it here. */
+return xfs_discard(s, aiocb->aio_offset, aiocb->aio_nbytes);
+}
+#endif
+
+/* If we couldn't manage to unmap while guaranteed that the area reads as
+ * all-zero afterwards, just write zeroes without unmapping */
+ret = handle_aiocb_write_zeroes(aiocb);
+return ret;
+}
+
 #ifndef HAVE_COPY_FILE_RANGE
 static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
  off_t *out_off, size_t len, unsigned int flags)
@@ -1732,6 +1761,9 @@ static int aio_worker(void *arg)
 case QEMU_AIO_WRITE_ZEROES:
 ret = handle_aiocb_write_zeroes(aiocb);
 break;
+case QEMU_AIO_WRITE_ZEROES | QEMU_AIO_DISCARD:
+ret = handle_aiocb_write_zeroes_unmap(aiocb);
+break;
 case QEMU_AIO_COPY_RANGE:
 ret = handle_aiocb_copy_range(aiocb);
 break;
@@ -2556,15 +2588,13 @@ static int coroutine_fn raw_co_pwrite_zeroes(
 int bytes, BdrvRequestFlags flags)
 {
 BDRVRawState *s = bs->opaque;
+int operation = QEMU_AIO_WRITE_ZEROES;
 
-if (!(flags & BDRV_REQ_MAY_UNMAP)) {
-return paio_submit_co(bs, s->fd, offset, NULL, bytes,
-  QEMU_AIO_WRITE_ZEROES);
-} else if (s->discard_zeroes) {
-return paio_submit_co(bs, s->fd, offset, NULL, bytes,
-  QEMU_AIO_DISCARD);
+if (flags & BDRV_REQ_MAY_UNMAP) {
+operation |= QEMU_AIO_DISCARD;
 }
-return -ENOTSUP;
+
+return paio_submit_co(bs, s->fd, offset, NULL, bytes, operation);
 }
 
 static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
@@ -3057,20 +3087,19 @@ static coroutine_fn int 
hdev_co_pwrite_zeroes(BlockDriverState *bs,
 int64_t offset, int bytes, BdrvRequestFlags flags)
 {
 BDRVRawState *s = bs->opaque;
+int operation = QEMU_AIO_WRITE_ZEROES | QEMU_AIO_BLKDEV;
 int rc;
 
 rc = fd_open(bs);
 if (rc < 0) {
 return rc;
 }
-if (!(flags & BDRV_REQ_MAY_UNMAP)) {
-return paio_submit_co(bs, s->fd, offset, NULL, bytes,
-  QEMU_AIO_WRITE_ZEROES|QEMU_AIO_BLKDEV);
-} else if (s->discard_zeroes) {
-return paio_submit_co(bs, s->fd, offset, NULL, bytes,
-  QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
+
+if (flags & BDRV_REQ_MAY_UNMAP) {
+operation |= QEMU_AIO_DISCARD;
 }
-return -ENOTSUP;
+
+return paio_submit_co(bs, s->fd, offset, NULL, bytes, operation);
 }
 
 static int coroutine_fn hdev_co_create_opts(const char *filename, QemuOpts 
*opts,
-- 
2.13.6

[Qemu-block] [PULL 03/13] file-posix: Handle EINTR in preallocation=full write

[Kevin Wolf]

From: Fam Zheng 

Cc: qemu-sta...@nongnu.org
Signed-off-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 block/file-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index ad299beb38..928b863ced 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1646,6 +1646,9 @@ static int handle_aiocb_truncate(RawPosixAIOData *aiocb)
 num = MIN(left, 65536);
 result = write(fd, buf, num);
 if (result < 0) {
+if (errno == EINTR) {
+continue;
+}
 result = -errno;
 error_setg_errno(errp, -result,
  "Could not write zeros for preallocation");
-- 
2.13.6

[Qemu-block] [PULL 13/13] qemu-iotests: Test query-blockstats with -drive and -blockdev

[Kevin Wolf]

Make sure that query-blockstats returns information for every
BlockBackend that is named or attached to a device model (or both).

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/227 | 101 ++
 tests/qemu-iotests/227.out | 205 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 307 insertions(+)
 create mode 100755 tests/qemu-iotests/227
 create mode 100644 tests/qemu-iotests/227.out

diff --git a/tests/qemu-iotests/227 b/tests/qemu-iotests/227
new file mode 100755
index 00..9a5f7f9f14
--- /dev/null
+++ b/tests/qemu-iotests/227
@@ -0,0 +1,101 @@
+#!/bin/bash
+#
+# Test query-blockstats with different ways to create a BB
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+here=$PWD
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+$QEMU -nographic -qmp-pretty stdio -serial none "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
+  | _filter_qemu | _filter_imgfmt \
+  | _filter_generated_node_ids
+}
+
+echo
+echo '=== blockstats with -drive if=virtio ==='
+echo
+
+run_qemu -drive driver=null-co,if=virtio <

[Qemu-block] [PULL 08/13] iotests: Add test for 'qemu-img convert -C' compatibility

[Kevin Wolf]

From: Fam Zheng 

Signed-off-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/082 |  8 
 tests/qemu-iotests/082.out | 11 +++
 2 files changed, 19 insertions(+)

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
index a872f771a6..3e605d52d1 100755
--- a/tests/qemu-iotests/082
+++ b/tests/qemu-iotests/082
@@ -158,6 +158,14 @@ run_qemu_img convert -o help
 run_qemu_img convert -O bochs -o help
 
 echo
+echo === convert: -C and other options ===
+
+# Adding the help option to a command without other -o options
+run_qemu_img convert -C -S 4k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+run_qemu_img convert -C -S 8k -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+run_qemu_img convert -C -c -O $IMGFMT "$TEST_IMG" "$TEST_IMG".target
+
+echo
 echo === amend: Options specified more than once ===
 
 # Last -f should win
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 60ef87c276..19e9fb13ff 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -508,6 +508,17 @@ size Virtual disk size
 Testing: convert -O bochs -o help
 qemu-img: Format driver 'bochs' does not support image creation
 
+=== convert: -C and other options ===
+
+Testing: convert -C -S 4k -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot enable copy offloading when -S is used
+
+Testing: convert -C -S 8k -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot enable copy offloading when -S is used
+
+Testing: convert -C -c -O qcow2 TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.target
+qemu-img: Cannot enable copy offloading when -c is used
+
 === amend: Options specified more than once ===
 
 Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2
-- 
2.13.6

[Qemu-block] [PULL 07/13] qemu-img: Add -C option for convert with copy offloading

[Kevin Wolf]

From: Fam Zheng 

Signed-off-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 qemu-img.c   | 21 +
 qemu-img-cmds.hx |  2 +-
 qemu-img.texi|  8 +++-
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 9b7506b8ae..1acddf693c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2024,11 +2024,12 @@ static int img_convert(int argc, char **argv)
  skip_create = false, progress = false, tgt_image_opts = false;
 int64_t ret = -EINVAL;
 bool force_share = false;
+bool explict_min_sparse = false;
 
 ImgConvertState s = (ImgConvertState) {
 /* Need at least 4k of zeros for sparse detection */
 .min_sparse = 8,
-.copy_range = true,
+.copy_range = false,
 .buf_sectors= IO_BUF_SIZE / BDRV_SECTOR_SIZE,
 .wr_in_order= true,
 .num_coroutines = 8,
@@ -2043,7 +2044,7 @@ static int img_convert(int argc, char **argv)
 {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
 {0, 0, 0, 0}
 };
-c = getopt_long(argc, argv, ":hf:O:B:co:l:S:pt:T:qnm:WU",
+c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
 long_options, NULL);
 if (c == -1) {
 break;
@@ -2067,9 +2068,11 @@ static int img_convert(int argc, char **argv)
 case 'B':
 out_baseimg = optarg;
 break;
+case 'C':
+s.copy_range = true;
+break;
 case 'c':
 s.compressed = true;
-s.copy_range = false;
 break;
 case 'o':
 if (!is_valid_option_list(optarg)) {
@@ -2112,7 +2115,7 @@ static int img_convert(int argc, char **argv)
 }
 
 s.min_sparse = sval / BDRV_SECTOR_SIZE;
-s.copy_range = false;
+explict_min_sparse = true;
 break;
 }
 case 'p':
@@ -2172,6 +2175,16 @@ static int img_convert(int argc, char **argv)
 goto fail_getopt;
 }
 
+if (s.compressed && s.copy_range) {
+error_report("Cannot enable copy offloading when -c is used");
+goto fail_getopt;
+}
+
+if (explict_min_sparse && s.copy_range) {
+error_report("Cannot enable copy offloading when -S is used");
+goto fail_getopt;
+}
+
 if (tgt_image_opts && !skip_create) {
 error_report("--target-image-opts requires use of -n flag");
 goto fail_getopt;
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 69758fb6e8..1526f327a5 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -44,7 +44,7 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
[-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B 
backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m 
num_coroutines] [-W] filename [filename2 [...]] output_filename")
+"convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
[-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B 
backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m 
num_coroutines] [-W] filename [filename2 [...]] output_filename")
 STEXI
 @item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] 
[-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l 
@var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] 
@var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
diff --git a/qemu-img.texi b/qemu-img.texi
index aeb1b9e66c..3b6710a580 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -169,6 +169,12 @@ Number of parallel coroutines for the convert process
 Allow out-of-order writes to the destination. This option improves performance,
 but is only recommended for preallocated devices like host devices or other
 raw block devices.
+@item -C
+Try to use copy offloading to move data from source image to target. This may
+improve performance if the data is remote, such as with NFS or iSCSI backends,
+but will not automatically sparsify zero sectors, and may result in a fully
+allocated target image depending on the host support for getting allocation
+information.
 @end table
 
 Parameters to dd subcommand:
@@ -319,7 +325,7 @@ Error on reading data
 
 @end table
 
-@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] 
[-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-l 
@var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] 
@var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fm

[Qemu-block] [PULL 12/13] block/qapi: Include anonymous BBs in query-blockstats

[Kevin Wolf]

Consistent with query-block, query-blockstats should not only include
named BlockBackends, but also those that are anonymous, but belong to a
device model.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 block/qapi.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/qapi.c b/block/qapi.c
index 50f867d634..339727f0f4 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -593,12 +593,16 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
 p_next = &info->next;
 }
 } else {
-for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
 BlockStatsList *info = g_malloc0(sizeof(*info));
 AioContext *ctx = blk_get_aio_context(blk);
 BlockStats *s;
 char *qdev;
 
+if (!*blk_name(blk) && !blk_get_attached_dev(blk)) {
+continue;
+}
+
 aio_context_acquire(ctx);
 s = bdrv_query_bds_stats(blk_bs(blk), true);
 s->has_device = true;
-- 
2.13.6

[Qemu-block] [PULL 09/13] block: Fix documentation for BDRV_REQ_MAY_UNMAP

[Kevin Wolf]

BDRV_REQ_MAY_UNMAP in a write_zeroes request does not only allow the
driver to unmap the blocks, but it actively requests that the blocks be
unmapped afterwards if at all possible.

Signed-off-by: Kevin Wolf 
---
 include/block/block.h | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index f85e3a6ed3..4e0871aaf9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -43,11 +43,12 @@ typedef struct BlockFragInfo {
 typedef enum {
 BDRV_REQ_COPY_ON_READ   = 0x1,
 BDRV_REQ_ZERO_WRITE = 0x2,
-/* The BDRV_REQ_MAY_UNMAP flag is used to indicate that the block driver
- * is allowed to optimize a write zeroes request by unmapping (discarding)
- * blocks if it is guaranteed that the result will read back as
- * zeroes. The flag is only passed to the driver if the block device is
- * opened with BDRV_O_UNMAP.
+
+/*
+ * The BDRV_REQ_MAY_UNMAP flag is used in write_zeroes requests to indicate
+ * that the block driver should unmap (discard) blocks if it is guaranteed
+ * that the result will read back as zeroes. The flag is only passed to the
+ * driver if the block device is opened with BDRV_O_UNMAP.
  */
 BDRV_REQ_MAY_UNMAP  = 0x4,
 
-- 
2.13.6

[Qemu-block] [PULL 02/13] qcow2: A grammar fix in conflicting cache sizing error message

[Kevin Wolf]

From: Leonid Bloch 

Signed-off-by: Leonid Bloch 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 block/qcow2.c  | 2 +-
 tests/qemu-iotests/103.out | 4 ++--
 tests/qemu-iotests/137.out | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6162ed8be2..ec9e6238a0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -797,7 +797,7 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts 
*opts,
 if (l2_cache_size_set && refcount_cache_size_set) {
 error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE
" and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set "
-   "the same time");
+   "at the same time");
 return;
 } else if (*l2_cache_size > combined_cache_size) {
 error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
diff --git a/tests/qemu-iotests/103.out b/tests/qemu-iotests/103.out
index bd45d3875a..bd9eec3250 100644
--- a/tests/qemu-iotests/103.out
+++ b/tests/qemu-iotests/103.out
@@ -5,10 +5,10 @@ wrote 65536/65536 bytes at offset 0
 
 === Testing invalid option combinations ===
 
-can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and 
refcount-cache-size may not be set the same time
+can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and 
refcount-cache-size may not be set at the same time
 can't open device TEST_DIR/t.IMGFMT: l2-cache-size may not exceed cache-size
 can't open device TEST_DIR/t.IMGFMT: refcount-cache-size may not exceed 
cache-size
-can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and 
refcount-cache-size may not be set the same time
+can't open device TEST_DIR/t.IMGFMT: cache-size, l2-cache-size and 
refcount-cache-size may not be set at the same time
 can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of 
two between 512 and the cluster size (65536)
 can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of 
two between 512 and the cluster size (65536)
 can't open device TEST_DIR/t.IMGFMT: L2 cache entry size must be a power of 
two between 512 and the cluster size (65536)
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
index 96724a6c33..6a2ffc71fd 100644
--- a/tests/qemu-iotests/137.out
+++ b/tests/qemu-iotests/137.out
@@ -16,7 +16,7 @@ read 33554432/33554432 bytes at offset 0
 === Try setting some invalid values ===
 
 Parameter 'lazy-refcounts' expects 'on' or 'off'
-cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+cache-size, l2-cache-size and refcount-cache-size may not be set at the same 
time
 l2-cache-size may not exceed cache-size
 refcount-cache-size may not exceed cache-size
 L2 cache size too big
-- 
2.13.6

[Qemu-block] [PULL 06/13] Revert "qemu-img: Document copy offloading implications with -S and -c"

[Kevin Wolf]

From: Fam Zheng 

This reverts commit eb461485f4558e362fab905735b50987505bca44.

Now that we introduce an explicit option, these implicit rules are not
used.

Signed-off-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 qemu-img.texi | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qemu-img.texi b/qemu-img.texi
index 5853cd18d1..aeb1b9e66c 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -96,8 +96,7 @@ will enumerate information about backing files in a disk 
image chain. Refer
 below for further description.
 
 @item -c
-indicates that target image must be compressed (qcow format only). If this
-option is used, copy offloading will not be attempted.
+indicates that target image must be compressed (qcow format only)
 
 @item -h
 with or without a command shows help and lists the supported formats
@@ -116,8 +115,7 @@ in case both @var{-q} and @var{-p} options are used.
 indicates the consecutive number of bytes that must contain only zeros
 for qemu-img to create a sparse image during conversion. This value is rounded
 down to the nearest 512 bytes. You may use the common size suffixes like
-@code{k} for kilobytes. If this option is used, copy offloading will not be
-attempted.
+@code{k} for kilobytes.
 
 @item -t @var{cache}
 specifies the cache mode that should be used with the (destination) file. See
-- 
2.13.6

[Qemu-block] [PULL 01/13] qcow: fix a reference leak

[Kevin Wolf]

From: KONRAD Frederic 

Since 42a3e1ab367cdf38cce093de24eb406b99a4ef96 qemu asserts when using the
vvfat driver:

git clone git://qemu.org/qemu.git
cd qemu
./configure --target-list=ppc-softmmu --enable-debug
make -j8
mkdir foo
touch foo/hello
./ppc-softmmu/qemu-system-ppc -M prep --nographic --monitor null \
  -hda fat:rw:./foo

"Ctrl-C"

qemu-system-ppc: block.c:3368: bdrv_close_all: Assertion \
   `((&all_bdrv_states)->tqh_first == ((void *)0))' failed.

This is because we reference bs twice in qcow_co_create(..) one time in
bdrv_open_blockdev_ref(..) and in blk_insert_bs(..) but we unref it only once
in blk_unref which leads to the reference leak.

Note that I didn't tested much QCOW after this change as I don't use it much.

Signed-off-by: KONRAD Frederic 
Signed-off-by: Kevin Wolf 
---
 block/qcow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow.c b/block/qcow.c
index 102d058d1c..385d935258 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -938,6 +938,7 @@ static int coroutine_fn 
qcow_co_create(BlockdevCreateOptions *opts,
 ret = 0;
 exit:
 blk_unref(qcow_blk);
+bdrv_unref(bs);
 qcrypto_block_free(crypto);
 return ret;
 }
-- 
2.13.6

[Qemu-block] [PULL 04/13] docs: Describe using images in writing iotests

[Kevin Wolf]

From: Fam Zheng 

Signed-off-by: Fam Zheng 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 docs/devel/testing.rst | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 5e19cd50da..8e1fa3a66e 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -255,6 +255,17 @@ comparable library support for invoking and interacting 
with QEMU programs. If
 you opt for Python, it is strongly recommended to write Python 3 compatible
 code.
 
+Both Python and Bash frameworks in iotests provide helpers to manage test
+images. They can be used to create and clean up images under the test
+directory. If no I/O or any protocol specific feature is needed, it is often
+more convenient to use the pseudo block driver, ``null-co://``, as the test
+image, which doesn't require image creation or cleaning up. Avoid system-wide
+devices or files whenever possible, such as ``/dev/null`` or ``/dev/zero``.
+Otherwise, image locking implications have to be considered.  For example,
+another application on the host may have locked the file, possibly leading to a
+test failure.  If using such devices are explicitly desired, consider adding
+``locking=off`` option to disable image locking.
+
 Docker based tests
 ==
 
-- 
2.13.6

[Qemu-block] [PULL 05/13] iotests: Don't lock /dev/null in 226

[Kevin Wolf]

From: Fam Zheng 

On my system (Fedora 28), this script reports a 'failed to get
"consistent read" lock' error. Following docs/devel/testing.rst, it's
better to add locking=off here.

Signed-off-by: Fam Zheng 
Reviewed-by: Eric Blake 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/226 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/226 b/tests/qemu-iotests/226
index 211ea9888a..8ec3e612dd 100755
--- a/tests/qemu-iotests/226
+++ b/tests/qemu-iotests/226
@@ -56,10 +56,10 @@ for PROTO in "file" "host_device" "host_cdrom"; do
 echo
 echo "== Testing RO =="
 $QEMU_IO -c "open -r -o driver=$PROTO,filename=$TEST_IMG" 2>&1 | 
_filter_testdir | _filter_imgfmt
-$QEMU_IO -c "open -r -o driver=$PROTO,filename=/dev/null" 2>&1 | 
_filter_imgfmt
+$QEMU_IO -c "open -r -o driver=$PROTO,filename=/dev/null,locking=off" 2>&1 
| _filter_imgfmt
 echo "== Testing RW =="
 $QEMU_IO -c "open -o driver=$PROTO,filename=$TEST_IMG" 2>&1 | 
_filter_testdir | _filter_imgfmt
-$QEMU_IO -c "open -o driver=$PROTO,filename=/dev/null" 2>&1 | 
_filter_imgfmt
+$QEMU_IO -c "open -o driver=$PROTO,filename=/dev/null,locking=off" 2>&1 | 
_filter_imgfmt
 done
 
 # success, all done
-- 
2.13.6

Re: [Qemu-block] interactive qemu-img

[Eric Blake]

On 07/28/2018 08:22 PM, Programmingkid wrote:

I thought of a way to make qemu-img much more user-friendly. When the user 
opens qemu-img without any arguments, we could present a prompt that guides the 
user on making an image file.

This illustrates what I think should happen.



Please select a format (qcow, qcow2, raw, vdi, vhdx, vmdk, vpc, vvfat):
qcow2

Please enter a size (e.g. 100M, 10G):
4G

Please enter a name:
WinXP.qcow2

Creating image file...done


The interactive prompt would contain enough options to make a usable image 
file. If the user wants to use some of the more advanced features of qemu-img 
he or she would still need to use the command-line.

Would such a patch be welcomed?


qemu-img is a command line tool, not a gui.  Bloating it with a gui 
dialog box is probably not a wise idea.


Personally, I'm just fine with the current command line behavior:

$ qemu-img
qemu-img: Not enough arguments
Try 'qemu-img --help' for more information

as 'qemu-img --help' tells you how to properly use the command, without 
having to hand-hold you through the process.


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

[Qemu-block] [PULL 00/13] Block layer patches

[Kevin Wolf]

The following changes since commit 6d9dd5fb9d0e9f4a174f53a0e20a39fbe809c71e:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qobject-2018-07-27-v2' 
into staging (2018-07-30 09:55:47 +0100)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 1239ac241fe170bb9fcf0be74bfff04f6f1c2560:

  qemu-iotests: Test query-blockstats with -drive and -blockdev (2018-07-30 
15:35:37 +0200)


Block layer patches:

- qemu-img convert -C is now required to enable copy offloading
- file-posix: Fix write_zeroes with unmap on block devices (would fall
  back to explicit writes on recent kernels)
- Fix query-blockstats interface for use with -blockdev
- Minor fixes and documentation updates


Fam Zheng (6):
  file-posix: Handle EINTR in preallocation=full write
  docs: Describe using images in writing iotests
  iotests: Don't lock /dev/null in 226
  Revert "qemu-img: Document copy offloading implications with -S and -c"
  qemu-img: Add -C option for convert with copy offloading
  iotests: Add test for 'qemu-img convert -C' compatibility

KONRAD Frederic (1):
  qcow: fix a reference leak

Kevin Wolf (5):
  block: Fix documentation for BDRV_REQ_MAY_UNMAP
  file-posix: Fix write_zeroes with unmap on block devices
  block/qapi: Add 'qdev' field to query-blockstats result
  block/qapi: Include anonymous BBs in query-blockstats
  qemu-iotests: Test query-blockstats with -drive and -blockdev

Leonid Bloch (1):
  qcow2: A grammar fix in conflicting cache sizing error message

 qapi/block-core.json   |  14 +++-
 include/block/block.h  |  11 +--
 block/file-posix.c |  62 ++
 block/qapi.c   |  16 +++-
 block/qcow.c   |   1 +
 block/qcow2.c  |   2 +-
 qemu-img.c |  21 -
 docs/devel/testing.rst |  11 +++
 qemu-img-cmds.hx   |   2 +-
 qemu-img.texi  |  14 ++--
 tests/qemu-iotests/082 |   8 ++
 tests/qemu-iotests/082.out |  11 +++
 tests/qemu-iotests/103.out |   4 +-
 tests/qemu-iotests/137.out |   2 +-
 tests/qemu-iotests/226 |   4 +-
 tests/qemu-iotests/227 | 101 ++
 tests/qemu-iotests/227.out | 205 +
 tests/qemu-iotests/group   |   1 +
 18 files changed, 449 insertions(+), 41 deletions(-)
 create mode 100755 tests/qemu-iotests/227
 create mode 100644 tests/qemu-iotests/227.out

Re: [Qemu-block] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Eric Blake]

On 07/28/2018 02:50 AM, Niels de Vos wrote:


Part of me wishes that libgfapi had just created a new function
'glfs_ftruncate2', so that existing users don't need to handle the api
change.  But I guess in the grand scheme, not a huge deal either way.


Gluster uses versioned symbols, so older binaries will keep working with
new libraries. It is (hopefully) rare that existing symbols get updated.
We try to send patches for these kind of changes to the projects we know
well in advance, reducing the number of surprises.



I can go ahead and add that to the comment in my branch after applying, if
Niels can let me know what that version is/will be (if known).


The new glfs_ftruncate() will be part of glusterfs-5 (planned for
October). We're changing the numbering scheme, it was expected to come
in glusterfs-4.2, but that is a version that never will be released.



Wait - so you're saying gluster has not yet released the incompatible 
change? Now would be the right time to get rid of the API breakage, 
before you bake it in, rather than relying solely on the versioned 
symbols to avoid an ABI breakage but forcing all clients to compensate 
to the API breakage.


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

Re: [Qemu-block] [PATCH 0/6] qcow2: Make the L2 cache cover the whole image by default

[Kevin Wolf]

Am 30.07.2018 um 14:13 hat Leonid Bloch geschrieben:
> On 07/30/2018 01:55 PM, Kevin Wolf wrote:
> > Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:
> > > This series makes the qcow2 L2 cache cover the entire image by default.
> > > The importance of this change is in noticeable performance improvement,
> > > especially with heavy random I/O. The memory overhead is very small:
> > > only 1 MB of cache for every 8 GB of image size. On systems with very
> > > limited RAM the maximal cache size can be limited by the existing
> > > cache-size and l2-cache-size options.
> > > 
> > > The L2 cache is also resized accordingly, by default, if the image is
> > > resized.
> > 
> > I agree with changing the defaults, I would have proposed a change
> > myself soon. We have been offering cache size options for a long time,
> > and management tools are still ignoring them. So we need to do something
> > in QEMU.
> > 
> > Now, what the exact defaults should be, is something to use a bit more
> > thought on. You say "the memory overhead is very small", without going
> > into details. Let's check.
> > 
> > This is the formula from your patch (unit is bytes):
> > 
> >  uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);
> > 
> > So we get:
> > 
> >  64k clusters, 8G image:   1M (maximum covered by old default)
> >  64k clusters, 1T image: 128M
> >  1M clusters, 1T image:8M
> >  512b clusters, 1T image: 16G
> > 
> > 1T is by far not the maximum size for a qcow2 image, and 16G memory
> > usage is definitely not "very small" overhead. Even the 128M for the
> > default cluster size are already a lot. So the simplistic approch of
> > this series won't work as a default.
> > 
> > Let's refine it a bit:
> > 
> > * Basing it on max_l2_cache sounds fine generally
> > * Cap it at 32 MB (?) by default
> 
> Yes! A great idea! Definitely necessary.
> 
> > * Enable cache-clean-interval=30 (?) by default to compensate a bit for
> >the higher maximum memory usage
> 
> Do you think that we need this if we cap the cache at 32 MB? And that's only
> the cap. 256 GB+ images are not that often used. And compared to the overall
> QEMU overhead, of over 500 MB, extra 32 in the worst case is not that much,
> considering the performance gain.

Don't forget that if you take a few snapshots so you get a backing file
chain, every layer in the chain has its own cache. So capping at 32 MB
with 9 backing files still means that we use 320 MB.

And honestly, is there a real reason not to use cache-clean-interval by
default? Even if it doesn't help much in some cases, it should hardly
ever hurt. If the image was completely idle for 30 seconds (or whatever
number we choose), having to reload some 64k from the disk on the next
access shouldn't make a big difference.

> > Another thing I just noticed while looking at the code is that
> > cache-clean-interval only considers blocks that aren't dirty, but
> > doesn't take measures to get dirty blocks written out, so we depend on
> > the guest flushing the cache before we can get free the memory. Should
> > we attempt to write unused dirty entries back? Berto?
> > 
> > > The reasons for making this behavior default, unlike in the patches I
> > > have sent previously, are the following:
> > > 1) Unelegant complications with making an option to accept either a
> > > size or a string, while outputting a proper error message.
> > > 2) More bulky logic to sort out what to do if the image is being resized
> > > but the (defined) overall cache size is too small to contain the
> > > new l2-cache-size.
> > > (Making this behavior default resolves all of these technical issues
> > > neatly)
> > 
> > The default must always correspond to some explicit setting. We can only
> > change defaults relatively freely because we can assume that if a user
> > cared about the setting, they would have specified it explicitly. If
> > it's not possible to specify a setting explicitly, we can't make this
> > assumption any more, so we'd be stuck with the default forever.
> > 
> > Now, considering what I wrote above, an alternate wouldn't be the best
> > way to represent this any more. We do have a cache size again (the 32 MB
> > cap) even while trying to cover the whole image.
> > 
> > Maybe the right interface with this in mind would be a boolean option
> > that specifies whether the given cache sizes are exact values (old
> > semantics) or maximum values, which are limited to what the actual
> > images size requires. If you do want the "real" full mode, you'd set
> > l2-cache-size=INT64_MAX and exact-cache-sizes=false (this could use a
> > better name). The new default would be l2-cache-size=32M,
> > exact-cache-sizes=false. The old default is cache-size=1M,
> > exact-cache-sizes=true.
> 
> The existing cache-size and l2-cache-size options are already documented as
> maximal values. It would make sense to actually make them as such: the
> caches will be as big as necessary to cove

Re: [Qemu-block] [PATCH 4/6] qcow2: Update total_sectors when resizing the image

[Kevin Wolf]

Am 30.07.2018 um 13:41 hat Leonid Bloch geschrieben:
> On 07/30/2018 12:43 PM, Kevin Wolf wrote:
> > Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:
> > > Signed-off-by: Leonid Bloch 
> > > ---
> > >   block/qcow2.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > index ec9e6238a0..223d351e40 100644
> > > --- a/block/qcow2.c
> > > +++ b/block/qcow2.c
> > > @@ -3646,6 +3646,8 @@ static int coroutine_fn 
> > > qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> > >   }
> > >   }
> > > +bs->total_sectors = offset / 512;
> > > +
> > >   /* write updated header.size */
> > >   offset = cpu_to_be64(offset);
> > >   ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
> > 
> > This shouldn't be necessary, bdrv_co_truncate() already updates
> > bs->total_sectors after calling the block driver.
> 
> It looks like without it the cache resize works only on the second resize.

Yes, and after reading the rest of the series, it makes sense to me
because qcow2_update_option() -> read_cache_sizes() uses
bs->total_sectors, so if we call that in qcow2_co_truncate(), we already
need to update the value early.

The comment should explain this connection because otherwise it's not
obvious when you read the code.

> > If this is needed by one of the following patches, we need a comment
> > that explains why this seemingly superfluous assignment is actually
> > necessary.
> > 
> > Also, 512 should be BDRV_SECTOR_SIZE.
> 
> I was surprised that it's not, but it's 512 also in two other places,
> including in qcow2_co_truncate itself. So I decided to keep that. Probably
> would be better if I'd repair it in the other places instead. :)

Yeah, or at least not introduce new places with a literal 512.

Kevin

Re: [Qemu-block] [PATCH 0/6] qcow2: Make the L2 cache cover the whole image by default

[Leonid Bloch]

On 07/30/2018 01:55 PM, Kevin Wolf wrote:

Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:

This series makes the qcow2 L2 cache cover the entire image by default.
The importance of this change is in noticeable performance improvement,
especially with heavy random I/O. The memory overhead is very small:
only 1 MB of cache for every 8 GB of image size. On systems with very
limited RAM the maximal cache size can be limited by the existing
cache-size and l2-cache-size options.

The L2 cache is also resized accordingly, by default, if the image is
resized.


I agree with changing the defaults, I would have proposed a change
myself soon. We have been offering cache size options for a long time,
and management tools are still ignoring them. So we need to do something
in QEMU.

Now, what the exact defaults should be, is something to use a bit more
thought on. You say "the memory overhead is very small", without going
into details. Let's check.

This is the formula from your patch (unit is bytes):

 uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);

So we get:

 64k clusters, 8G image:   1M (maximum covered by old default)
 64k clusters, 1T image: 128M
 1M clusters, 1T image:8M
 512b clusters, 1T image: 16G

1T is by far not the maximum size for a qcow2 image, and 16G memory
usage is definitely not "very small" overhead. Even the 128M for the
default cluster size are already a lot. So the simplistic approch of
this series won't work as a default.

Let's refine it a bit:

* Basing it on max_l2_cache sounds fine generally
* Cap it at 32 MB (?) by default


Yes! A great idea! Definitely necessary.


* Enable cache-clean-interval=30 (?) by default to compensate a bit for
   the higher maximum memory usage


Do you think that we need this if we cap the cache at 32 MB? And that's 
only the cap. 256 GB+ images are not that often used. And compared to 
the overall QEMU overhead, of over 500 MB, extra 32 in the worst case is 
not that much, considering the performance gain.



Another thing I just noticed while looking at the code is that
cache-clean-interval only considers blocks that aren't dirty, but
doesn't take measures to get dirty blocks written out, so we depend on
the guest flushing the cache before we can get free the memory. Should
we attempt to write unused dirty entries back? Berto?


The reasons for making this behavior default, unlike in the patches I
have sent previously, are the following:
1) Unelegant complications with making an option to accept either a
size or a string, while outputting a proper error message.
2) More bulky logic to sort out what to do if the image is being resized
but the (defined) overall cache size is too small to contain the
new l2-cache-size.
(Making this behavior default resolves all of these technical issues
neatly)


The default must always correspond to some explicit setting. We can only
change defaults relatively freely because we can assume that if a user
cared about the setting, they would have specified it explicitly. If
it's not possible to specify a setting explicitly, we can't make this
assumption any more, so we'd be stuck with the default forever.

Now, considering what I wrote above, an alternate wouldn't be the best
way to represent this any more. We do have a cache size again (the 32 MB
cap) even while trying to cover the whole image.

Maybe the right interface with this in mind would be a boolean option
that specifies whether the given cache sizes are exact values (old
semantics) or maximum values, which are limited to what the actual
images size requires. If you do want the "real" full mode, you'd set
l2-cache-size=INT64_MAX and exact-cache-sizes=false (this could use a
better name). The new default would be l2-cache-size=32M,
exact-cache-sizes=false. The old default is cache-size=1M,
exact-cache-sizes=true.


The existing cache-size and l2-cache-size options are already documented 
as maximal values. It would make sense to actually make them as such: 
the caches will be as big as necessary to cover the entire image (no 
need for them to be more than that) but they will be capped by the 
current options, while the new default of l2-cache-size will be 32M. Why 
would one need exact-cache-sizes, if they would be MIN(needed, max)?

Does it sound reasonable?

Leonid.



Kevin


3) The performance gain (as measured by fio in random read/write tests)
can be as high as 50%, or even more, so this would be a reasonable
default behavior.
4) The memory overhead is really small for the gain, and in cases when
memory economy is critical, the maximal cache values can always be
set by the appropriate options.

Re: [Qemu-block] [PATCH 4/6] qcow2: Update total_sectors when resizing the image

[Leonid Bloch]

On 07/30/2018 12:43 PM, Kevin Wolf wrote:

Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:

Signed-off-by: Leonid Bloch 
---
  block/qcow2.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..223d351e40 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3646,6 +3646,8 @@ static int coroutine_fn 
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
  }
  }
  
+bs->total_sectors = offset / 512;

+
  /* write updated header.size */
  offset = cpu_to_be64(offset);
  ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),


This shouldn't be necessary, bdrv_co_truncate() already updates
bs->total_sectors after calling the block driver.


It looks like without it the cache resize works only on the second resize.


If this is needed by one of the following patches, we need a comment
that explains why this seemingly superfluous assignment is actually
necessary.

Also, 512 should be BDRV_SECTOR_SIZE.


I was surprised that it's not, but it's 512 also in two other places, 
including in qcow2_co_truncate itself. So I decided to keep that. 
Probably would be better if I'd repair it in the other places instead. :)


Thanks!
Leonid.


Kevin

Re: [Qemu-block] [PATCH for-3.0 1/3] block/qapi: Add 'qdev' field to query-blockstats result

[Peter Krempa]

On Fri, Jul 27, 2018 at 17:58:54 +0200, Kevin Wolf wrote:
> Am 27.07.2018 um 17:07 hat Eric Blake geschrieben:
> > On 07/27/2018 09:15 AM, Kevin Wolf wrote:
> > > Like for query-block, the client needs to identify which BlockBackend
> > > the returned data is for. Anonymous BlockBackends are identified by the
> > > device model they are attached to. Add a 'qdev' field that contains the
> > > qdev ID or QOM path of the attached device model.
> > > 
> > > Signed-off-by: Kevin Wolf 
> > > ---
> > 
> > > @@ -879,7 +882,7 @@
> > >   # Since: 0.14.0
> > >   ##
> > >   { 'struct': 'BlockStats',
> > > -  'data': {'*device': 'str', '*node-name': 'str',
> > > +  'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
> > >  'stats': 'BlockDeviceStats',
> > >  '*parent': 'BlockStats',
> > >  '*backing': 'BlockStats'} }
> > 
> > Can we also update the example under query-blockstats a few lines later to
> > show the added field?
> 
> I'll add the qdev field, but the example is hopelessly outdated anyway.
> Sounds like something for another patch.

When doing further cleanups it would be worth also noting that the
'query-nodes' argument of 'query-blockstats' does not return correct
information.

Right now it creates a false impression that it just changes the output
from nested to linear and includes filter nodes, but in fact the data is
not correct in that mode.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 0/6] qcow2: Make the L2 cache cover the whole image by default

[Kevin Wolf]

Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:
> This series makes the qcow2 L2 cache cover the entire image by default.
> The importance of this change is in noticeable performance improvement,
> especially with heavy random I/O. The memory overhead is very small:
> only 1 MB of cache for every 8 GB of image size. On systems with very
> limited RAM the maximal cache size can be limited by the existing
> cache-size and l2-cache-size options.
> 
> The L2 cache is also resized accordingly, by default, if the image is
> resized.

I agree with changing the defaults, I would have proposed a change
myself soon. We have been offering cache size options for a long time,
and management tools are still ignoring them. So we need to do something
in QEMU.

Now, what the exact defaults should be, is something to use a bit more
thought on. You say "the memory overhead is very small", without going
into details. Let's check.

This is the formula from your patch (unit is bytes):

uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);

So we get:

64k clusters, 8G image:   1M (maximum covered by old default)
64k clusters, 1T image: 128M
1M clusters, 1T image:8M
512b clusters, 1T image: 16G

1T is by far not the maximum size for a qcow2 image, and 16G memory
usage is definitely not "very small" overhead. Even the 128M for the
default cluster size are already a lot. So the simplistic approch of
this series won't work as a default.

Let's refine it a bit:

* Basing it on max_l2_cache sounds fine generally
* Cap it at 32 MB (?) by default
* Enable cache-clean-interval=30 (?) by default to compensate a bit for
  the higher maximum memory usage

Another thing I just noticed while looking at the code is that
cache-clean-interval only considers blocks that aren't dirty, but
doesn't take measures to get dirty blocks written out, so we depend on
the guest flushing the cache before we can get free the memory. Should
we attempt to write unused dirty entries back? Berto?

> The reasons for making this behavior default, unlike in the patches I
> have sent previously, are the following:
> 1) Unelegant complications with making an option to accept either a
>size or a string, while outputting a proper error message.
> 2) More bulky logic to sort out what to do if the image is being resized
>but the (defined) overall cache size is too small to contain the
>new l2-cache-size.
> (Making this behavior default resolves all of these technical issues
> neatly)

The default must always correspond to some explicit setting. We can only
change defaults relatively freely because we can assume that if a user
cared about the setting, they would have specified it explicitly. If
it's not possible to specify a setting explicitly, we can't make this
assumption any more, so we'd be stuck with the default forever.

Now, considering what I wrote above, an alternate wouldn't be the best
way to represent this any more. We do have a cache size again (the 32 MB
cap) even while trying to cover the whole image.

Maybe the right interface with this in mind would be a boolean option
that specifies whether the given cache sizes are exact values (old
semantics) or maximum values, which are limited to what the actual
images size requires. If you do want the "real" full mode, you'd set
l2-cache-size=INT64_MAX and exact-cache-sizes=false (this could use a
better name). The new default would be l2-cache-size=32M,
exact-cache-sizes=false. The old default is cache-size=1M,
exact-cache-sizes=true.

Kevin

> 3) The performance gain (as measured by fio in random read/write tests)
>can be as high as 50%, or even more, so this would be a reasonable
>default behavior.
> 4) The memory overhead is really small for the gain, and in cases when
>memory economy is critical, the maximal cache values can always be
>set by the appropriate options.

Re: [Qemu-block] [PATCH 4/6] qcow2: Update total_sectors when resizing the image

[Kevin Wolf]

Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben:
> Signed-off-by: Leonid Bloch 
> ---
>  block/qcow2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ec9e6238a0..223d351e40 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3646,6 +3646,8 @@ static int coroutine_fn 
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>  }
>  }
>  
> +bs->total_sectors = offset / 512;
> +
>  /* write updated header.size */
>  offset = cpu_to_be64(offset);
>  ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),

This shouldn't be necessary, bdrv_co_truncate() already updates
bs->total_sectors after calling the block driver.

If this is needed by one of the following patches, we need a comment
that explains why this seemingly superfluous assignment is actually
necessary.

Also, 512 should be BDRV_SECTOR_SIZE.

Kevin

Re: [Qemu-block] [Qemu-devel] [RFC PATCH] rbd: Don't convert keypairs to JSON and back

[Markus Armbruster]

Jeff Cody  writes:

> On Wed, Jul 25, 2018 at 10:56:48AM -0500, Eric Blake wrote:
>> On 07/25/2018 10:10 AM, Markus Armbruster wrote:
>> >qemu_rbd_parse_filename() builds a keypairs QList, converts it to JSON, and
>> >stores the resulting QString in a QDict.
>> >
>> >qemu_rbd_co_create_opts() and qemu_rbd_open() get the QString from the
>> >QDict, pass it to qemu_rbd_set_keypairs(), which converts it back into
>> >a QList.
>> >
>> >Drop both conversions, store the QList instead.
>> >
>> >This affects output of qemu-img info.  Before this patch:
>> >
>> > $ qemu-img info 
>> > rbd:rbd/testimg.raw:mon_host=192.168.15.180:rbd_cache=true:conf=/tmp/ceph.conf
>> > [junk printed by Ceph library code...]
>> > image: json:{"driver": "raw", "file": {"pool": "rbd", "image": 
>> > "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", 
>> > "=keyvalue-pairs": "[\"mon_host\", \"192.168.15.180\", \"rbd_cache\", 
>> > \"true\"]"}}
>> > [more output, not interesting here]
>> >
>> >After this patch, we get
>> >
>> > image: json:{"driver": "raw", "file": {"pool": "rbd", "image": 
>> > "testimg.raw", "conf": "/tmp/ceph.conf", "driver": "rbd", 
>> > "=keyvalue-pairs": ["mon_host", "192.168.15.180", "rbd_cache", "true"]}}
>> >
>> >The value of member "=keyvalue-pairs" changes from a string containing
>> >a JSON array to that JSON array.  That's an improvement of sorts.  However:
>> >
>> >* Should "=keyvalue-pairs" even be visible here?  It's supposed to be
>> >   purely internal...
>> 
>> I'd argue that since it is supposed to be internal (as evidenced by the
>> leading '=' that does not name a normal variable), changing it doesn't hurt
>> stability. But yes, it would be nicer if we could filter it entirely so that
>> it does not appear in json: output, if it doesn't truly affect the contents
>> that the guest would see.
>> 
>> >
>> >* Is this a stable interface we need to preserve, warts and all?
>> 
>> I hope not.
>> 
>> >
>> >Signed-off-by: Markus Armbruster 
>> >---
>> >  block/rbd.c | 50 ++
>> >  1 file changed, 22 insertions(+), 28 deletions(-)
>> 
>> I'm not yet convinced if we want this patch for 3.0 without more comments
>> from the RBD experts, nor do I see too much of an issue if this doesn't go
>> in until 3.1.  But as to the code changes itself, I find them nice.
>
> Based on my IRC discussions with Markus, I believe the target for this patch
> is indeed 3.1, not 3.0.

Unless we conclude we want to change qemu-img info sooner rather than
later, which seems unlikely.

Re: [Qemu-block] [PATCH 0/2] Refine some vdi code

[yuchenlin]

Hi, Stefan

I agree that redundancy of If else may helps people to understand the code.

However, CONFIG_VDI_WRITE only contributes:

#if defined(CONFIG_VDI_WRITE)
.bdrv_co_pwritev = vdi_co_pwritev,
#endif

I think we don't need CONFIG_VDI_WRITE to document the code.
As its name implies, vdi_co_pwritev shows the code parts for vdi write support.


I appreciated your time and effort for reviews.

Regards,
yuchenlin


Stefan Weil  於 2018-07-30 15:13 寫道：
> Am 30.07.2018 um 04:46 schrieb yuchen...@synology.com: > From: yuchenlin 
>  > > This series refine some code in vdi.c, includes: 
> > > * Remvoe CONFIG_VDI_WRITE because there is no reason to leave an always 
> on > and cannot configure option in the code-side. > * decouple if else if 
> chain to get more readability. > > Thanks, > yuchenlin > > yuchenlin (2): > 
> vdi: remove CONFIG_VDI_WRITE > vdi: refine code for vdi_open > > block/vdi.c 
> | 32 ++-- > 1 file changed, 18 insertions(+), 14 
> deletions(-) Technically these changes are fine, but personally I prefer my 
> old code. If else is rendundant here, but redundancy helps humans to 
> understand the code. CONFIG_VDI_WRITE still has a similar function as it 
> documents which code parts are relevant for write support. Stefan   

Re: [Qemu-block] [PATCH 0/2] Refine some vdi code

[Stefan Weil]

Am 30.07.2018 um 04:46 schrieb yuchen...@synology.com:
> From: yuchenlin 
> 
> This series refine some code in vdi.c, includes:
> 
> * Remvoe CONFIG_VDI_WRITE because there is no reason to leave an always on
>   and cannot configure option in the code-side.
> * decouple if else if chain to get more readability.
> 
> Thanks,
> yuchenlin
> 
> yuchenlin (2):
>   vdi: remove CONFIG_VDI_WRITE
>   vdi: refine code for vdi_open
> 
>  block/vdi.c | 32 ++--
>  1 file changed, 18 insertions(+), 14 deletions(-)


Technically these changes are fine, but personally I prefer my old code.
If else is rendundant here, but redundancy helps humans to understand
the code. CONFIG_VDI_WRITE still has a similar function as it documents
which code parts are relevant for write support.

Stefan