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

[Programmingkid]

> On Jul 30, 2018, at 3:39 PM, Max Reitz <address@hidden> 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 <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.

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 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.

> 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] ".

That does sound like a good idea.

> (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.

I'm guessing a numbered list of options is what you want.
Example:

1). Option 1
2). Option 2
3). Option 3

> 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.

Good point. I could write something nice in Python.

> 
> 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);
>> 
> 
>