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 <address@hidden>
> ---
>  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