Re: [PATCH v2 2/2] virsh: Allow users to reedit rejected XML

Eric Blake <eblake@xxxxxxxxxx> · Thu, 31 May 2012 13:32:21 -0600

On 05/24/2012 10:20 AM, Michal Privoznik wrote:
> If users *-edit but make a mistake in XML all changes are
> permanently lost. However, if virsh is not running within
> a script we can ask user if he wants to re-edit the file
> and correct the mistakes.
> ---
>  tools/console.c    |   40 +++++++++++++++++++++------------
>  tools/console.h    |    2 +
>  tools/virsh-edit.c |   39 +++++++++++++++++++++++++++-----
>  tools/virsh.c      |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 122 insertions(+), 21 deletions(-)
> 

> +static int
> +vshAskReedit(vshControl *ctl, const char *msg)
> +{
> +#ifndef WIN32
> +    int c = -1;
> +    struct termios ttyattr;
> +
> +    if (!isatty(STDIN_FILENO))
> +        return -1;
> +
> +    virshReportError(ctl);
> +
> +    if (vshMakeStdinRaw(&ttyattr, false) < 0)
> +        return -1;
> +
> +    while (true) {
> +        /* TRANSLATORS: For now, we aren't using LC_MESSAGES, and the user
> +         * choices really are limited to just 'y', 'n', 'f' and '?'  */
> +        vshPrint(ctl, "\r%s %s", msg, _("Try again? [y,n,f,?]:"));
> +        c = getchar();
> +        c = c_tolower(c);

It should be safe to combine these into one statement:

c = c_tolower(getchar());

but that's not strictly necessary.

> +
> +        if (c == '?') {
> +            vshPrint(ctl, "\r\n%s", _("y - yes, start editor again\r\n"
> +                                      "n - no, throw away my changes\r\n"
> +                                      "f - force, try to redefine again\r\n"
> +                                      "? - print this help\r\n"));
> +            continue;
> +        } else if (c == 'y' || c == 'n' || c == 'f') {
> +            break;
> +        }
> +    }
> +
> +    tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr);
> +
> +    if (c == 'N')
> +        goto cleanup;

Dead if statement; you can't get here when c=='N', and even if you could...

> +
> +cleanup:

...you end up at the same label.

> +    vshPrint(ctl, "\r\n");
> +    return c;
> +#else
> +    vshDebug(ctl, VSH_ERR_WARNING, "%s", _("This function is not "
> +                                           "supported on WIN32 platform"));
> +    return 0;
> +#endif
> +}

ACK with the dead code cleaned up, although it might be worth reposting
on top of your v3 cleanup to 1/2 to make sure your fixes there don't
cause problems here.

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org