[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 18.05.2012 19:27, Eric Blake wrote:
> On 05/18/2012 06:48 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 as user if he wants to re-edit the file
> 
> s/as/ask/
> 
>> and correct the mistakes.
>> ---
>>  tools/console.c    |   40 +++++++++++++++++++++++++---------------
>>  tools/console.h    |    2 ++
>>  tools/virsh-edit.c |    8 +++++++-
>>  tools/virsh.c      |   45 +++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/tools/console.c b/tools/console.c
>> index 34fde05..90e54e3 100644
>> --- a/tools/console.c
>> +++ b/tools/console.c
>> @@ -298,13 +298,36 @@ vshGetEscapeChar(const char *s)
>>      return *s;
>>  }
>>  
>> +int vshMakeStdinRaw(struct termios *ttyattr, bool report_errors) {
> 
> [1] This function will not be compiled on WIN32...
> 
> 
>> +    if (vshMakeStdinRaw(&ttyattr, true) < 0)
>>          goto resettty;
> 
> [2] Should you refactor the resettty code into a helper function?
> 
>> +++ b/tools/virsh-edit.c
>> @@ -14,6 +14,7 @@ do {
>>      if (!tmp)
>>          goto edit_cleanup;
>>  
>> +reedit:
>>      /* Start the editor. */
>>      if (editFile (ctl, tmp) == -1)
>>          goto edit_cleanup;
>> @@ -45,8 +46,13 @@ do {
>>      }
>>  
>>      /* Everything checks out, so redefine the domain. */
>> -    if (!(EDIT_DEFINE))
>> +    if (!(EDIT_DEFINE)) {
>> +        /* Redefine failed. If we are not running within
>> +         * a script ask used if he wants to re-edit the XML */
>> +        if (vshAskReedit(ctl) > 0)
>> +            goto reedit;
> 
> Do we want to change behavior based on result of <0 (errored out trying
> to read tty) vs. ==0 (successfully learned the answer is no)?
> 
>>  
>> +/**
>> + * vshAskReedit:
>> + *
>> + * Ask user if he wants to return to previously
>> + * edited file.
>> + *
>> + * Returns  1 if he wants to
>> + *          0 if he doesn't want to
>> + *         -1 on error
>> + */
>> +static int
>> +vshAskReedit(vshControl *ctl)
>> +{
>> +    int ret = 0;
>> +    int c = 0;
>> +    struct termios ttyattr;
>> +
>> +    if (!isatty(STDIN_FILENO))
>> +        return -1;
> 
> Is this really a fatal error, or should we be returning 0 here as if the
> user had always answered no?
> 
>> +
>> +    virshReportError(ctl);
>> +
>> +    if (vshMakeStdinRaw(&ttyattr, false) < 0)
> 
> [1] ...you are blindly calling it from all platforms here.  You need to
> fix mingw compilation.

Okay, on mingw I've make vshAskReedit function return always 0.
> 
>> +        return -1;
> 
> [2] you are returning without restoring the tty to a sane state.  Oops.

I don't think so. If vshMakeStdinRaw returns -1 it was unable to make
stdin raw.

> 
>> +
>> +    while (true) {
>> +        vshPrint(ctl, "\rFailed. Try again? (y/Y/n/N) [Y]:");
> 
> Mark this string for translation with _().  Actually, you _don't_ want
> to mark \r for translation.  Also, by marking this for translation, I'm
> a bit worried that translators may substitute other common characters
> for their language, only to be disappointed.  So this should be
> something like the following, including the magic gettext comment:
> 
>   /* TRANSLATORS: For now, we aren't using LC_MESSAGES, and the user
>      choices really are limited to just 'y' and 'n'.  */
> vshPrintf(ctl, "\r%s", _("Failed. Try again..."));

Well, if I take into account your last e-mail, how should this message
look like? I mean - how offer users 3 choices with intuitive names hence
shortcuts?

Failed. [R]eedit/[S]tart over again/[Q]uit?
> 
>> +        c = getchar();
>> +        c = c_toupper(c);
>> +        if (c == '\n' || c == '\r' || c == 'Y' || c == 'N')
>> +            break;
> 
> This turns 'maybe' into yes, and 'undecided' into no, in violation of
> the POSIX requirements on 'yesexpr' and 'noexpr' from LC_MESSAGES (in
> the POSIX locale, the winning character must be the first on a line).
> Furthermore, in non-English locales, it might even do the wrong thing.
> Too bad gnulib's 'yesno' and 'rpmatch' modules are GPLv3, but ideally,
> we should be testing for a regex match to the LC_MESSAGES pattern for
> the current locale.
> 
> That said, I18N is complex enough that I'm okay saving it for a later
> patch, and won't insist that we get it right for the initial checkin
> (that is, any re-editing abilities, even if English-only, are better
> than none).

okay

> 
> [Note - technically virsh can be GPL while the rest of libvirt.so is
> LGPL, if we really wanted to pull in GPL gnulib modules just for the
> command line interface, but then you can't copy flies out of virsh into
> the rest of libvirt, so in the past, we have decided not to take on that
> risk]
> 
>> +    }
>> +
>> +    tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr);
>> +
>> +    if (c == 'N')
>> +        goto cleanup;
>> +
>> +    ret = 1;
>> +cleanup:
>> +    vshPrint(ctl, "\r\n");
>> +    return ret;
>> +}
>> +
>>  /* ---------------
>>   * Commands
>>   * ---------------
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]