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 <programmingk...@gmail.com> > --- > 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_mode(); > } > > qemu_add_opts(&qemu_object_opts); >
signature.asc
Description: OpenPGP digital signature