Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command
> On Jul 31, 2018, at 6:48 AM, Kevin Wolf wrote: > > Am 30.07.2018 um 22:27 hat Programmingkid geschrieben: >> On Jul 30, 2018, at 3:39 PM, Max Reitz wrote: >>> On 2018-07-30 21:14, John Arbuckle wrote: >>> 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. > > We do have things like the QMP shell in scripts/, so I don't think > having this script in the QEMU git tree is completely out of question, > but I agree that it's not obvious that we should do it. > >> I got the idea of the interactive mode from bochs. It has a very extensive >> set of options that it walks the user thru. I really liked how complete >> it was. A management tool would be helpful to the user, but it wouldn't >> be included with qemu-img. That was my goal. > > That's my biggest problem with your approach: It's not complete at all. > It only provides the most basic options without which image creation > would fail. The three questions are enough to make an image file. I made a image file of each of the formats that are listed in the patch. > But the real advantage in an interactive user interface would be to make > advanced options more discoverable. All of them. As long as you don't > provide that, it's useless. I won't say useless. There are plenty of people who use it to create image files. But it is very limited in what it can do interactively. Having the user be able to select any one of the commands available to qemu-img would make this patch more useful. > Everybody knows how to do the basic > 'qemu-img create -f qcow2 test.qcow2 8G' (and if they don't, they'll > know after reading the help text or searching the internet for 10 > seconds). I want to make qemu-img easy enough to use that googling how to use it becomes a thing of the past. > Also, it's completely unclear to me why 'qemu-img' without any arguments > should mean creating an image and not one of the other operations. I agree now. This patch should provide the user with the full list of commands available. > > Kevin Thank you for reviewing my patch.
Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command
Am 30.07.2018 um 22:27 hat Programmingkid geschrieben: > On Jul 30, 2018, at 3:39 PM, Max Reitz wrote: > > On 2018-07-30 21:14, John Arbuckle wrote: > > 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. We do have things like the QMP shell in scripts/, so I don't think having this script in the QEMU git tree is completely out of question, but I agree that it's not obvious that we should do it. > I got the idea of the interactive mode from bochs. It has a very extensive > set of options that it walks the user thru. I really liked how complete > it was. A management tool would be helpful to the user, but it wouldn't > be included with qemu-img. That was my goal. That's my biggest problem with your approach: It's not complete at all. It only provides the most basic options without which image creation would fail. But the real advantage in an interactive user interface would be to make advanced options more discoverable. All of them. As long as you don't provide that, it's useless. Everybody knows how to do the basic 'qemu-img create -f qcow2 test.qcow2 8G' (and if they don't, they'll know after reading the help text or searching the internet for 10 seconds). Also, it's completely unclear to me why 'qemu-img' without any arguments should mean creating an image and not one of the other operations. Kevin
Re: [Qemu-block] [PATCH] Add interactive mode to qemu-img command
> 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
> 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] [PATCH] Add interactive mode to qemu-img command
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] [PATCH] Add interactive mode to qemu-img command
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
[Qemu-block] [PATCH] Add interactive mode to qemu-img command
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)
