4:49 a.m.
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
> * ---------------