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
>> * ---------------
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list