[libvirt] [PATCH v2] virsh-edit: Make force editing usable
When editing a domain with 'virsh edit' and failing validation, the usual message pops up: Failed. Try again? [y,n,f,?]: Turning of validation can be ussable, mainly for testing (but other purposes too), so this patch adds support for relaxing definition in virsh-edit and makes 'virsh edit <domain>' more usable. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - make a special case 'v' for turning off validation tools/virsh-domain.c | 6 ++++++ tools/virsh-edit.c | 21 +++++++++++++++++++-- tools/virsh.c | 28 +++++++++++++++++++++------- tools/virsh.h | 4 ++-- 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2506b89..b4761c6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11265,7 +11265,13 @@ cmdEdit(vshControl *ctl, const vshCmd *cmd) } while (0) #define EDIT_DEFINE \ (dom_edited = vshDomainDefine(ctl->conn, doc_edited, define_flags)) +#define EDIT_RELAX \ + do { \ + define_flags &= ~VIR_DOMAIN_DEFINE_VALIDATE; \ + } while (0); + #include "virsh-edit.c" +#undefine EDIT_RELAX vshPrint(ctl, _("Domain %s XML configuration edited.\n"), virDomainGetName(dom_edited)); diff --git a/tools/virsh-edit.c b/tools/virsh-edit.c index 41a6d53..2a13a04 100644 --- a/tools/virsh-edit.c +++ b/tools/virsh-edit.c @@ -1,7 +1,7 @@ /* * virsh-edit.c: Implementation of generic virsh *-edit intelligence * - * Copyright (C) 2012 Red Hat, Inc. + * Copyright (C) 2012, 2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -62,6 +62,7 @@ do { char *doc_reread = NULL; const char *msg = NULL; bool edit_success = false; + bool relax_avail = false; /* Get the XML configuration of the object. */ doc = (EDIT_GET_XML); @@ -74,6 +75,11 @@ do { goto edit_cleanup; reedit: + +#ifdef EDIT_RELAX + relax_avail = true; +#endif + /* Start the editor. */ if (vshEditFile(ctl, tmp) == -1) goto edit_cleanup; @@ -112,7 +118,7 @@ do { msg = _("Failed."); if (msg) { - int c = vshAskReedit(ctl, msg); + int c = vshAskReedit(ctl, msg, relax_avail); switch (c) { case 'y': goto reedit; @@ -126,6 +132,17 @@ do { goto edit_cleanup; break; +#ifdef EDIT_RELAX + case 'v': + if (relax_avail) { + EDIT_RELAX; + relax_avail = false; + goto redefine; + break; + } + /* fall-through */ +#endif + default: vshError(ctl, "%s", msg); break; diff --git a/tools/virsh.c b/tools/virsh.c index aba34ce..464af3d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1,7 +1,7 @@ /* * virsh.c: a shell to exercise the libvirt API * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -512,13 +512,14 @@ vshPrintRaw(vshControl *ctl, ...) * edited file. * * Returns 'y' if he wants to - * 'f' if he forcibly wants to * 'n' if he doesn't want to + * 'v' if he wants to try defining it again with validation turned off + * 'f' if he forcibly wants to * -1 on error * 0 otherwise */ int -vshAskReedit(vshControl *ctl, const char *msg) +vshAskReedit(vshControl *ctl, const char *msg, bool relax_avail) { int c = -1; @@ -532,8 +533,9 @@ vshAskReedit(vshControl *ctl, const char *msg) 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,?]:")); + * choices really are limited to just 'y', 'n', 'v', 'f' and '?' */ + vshPrint(ctl, _("\r%s Try again? %s: "), msg, + relax_avail ? "[y,n,v,f,?]" : "[y,n,f,?]"); c = c_tolower(getchar()); if (c == '?') { @@ -541,11 +543,21 @@ vshAskReedit(vshControl *ctl, const char *msg) "", _("y - yes, start editor again"), _("n - no, throw away my changes"), + NULL); + + if (relax_avail) { + vshPrintRaw(ctl, + _("v - turn off validation and try to redefine again"), + NULL); + } + + vshPrintRaw(ctl, _("f - force, try to redefine again"), _("? - print this help"), NULL); continue; - } else if (c == 'y' || c == 'n' || c == 'f') { + } else if (c == 'y' || c == 'n' || c == 'f' || + (relax_avail && c == 'v')) { break; } } @@ -557,7 +569,9 @@ vshAskReedit(vshControl *ctl, const char *msg) } #else /* WIN32 */ int -vshAskReedit(vshControl *ctl, const char *msg ATTRIBUTE_UNUSED) +vshAskReedit(vshControl *ctl, + const char *msg ATTRIBUTE_UNUSED, + bool relax_avail ATTRIBUTE_UNUSED) { vshDebug(ctl, VSH_ERR_WARNING, "%s", _("This function is not " "supported on WIN32 platform")); diff --git a/tools/virsh.h b/tools/virsh.h index 7d5d8a2..df2ea7f 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -1,7 +1,7 @@ /* * virsh.h: a shell to exercise the libvirt API * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -359,7 +359,7 @@ char *vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item) char *vshEditWriteToTempFile(vshControl *ctl, const char *doc); int vshEditFile(vshControl *ctl, const char *filename); char *vshEditReadBackFile(vshControl *ctl, const char *filename); -int vshAskReedit(vshControl *ctl, const char *msg); +int vshAskReedit(vshControl *ctl, const char *msg, bool relax_avail); int vshStreamSink(virStreamPtr st, const char *bytes, size_t nbytes, void *opaque); double vshPrettyCapacity(unsigned long long val, const char **unit); -- 2.3.0
On Thu, Feb 19, 2015 at 05:24:22PM +0100, Martin Kletzander wrote:
When editing a domain with 'virsh edit' and failing validation, the usual message pops up:
Failed. Try again? [y,n,f,?]:
Turning of validation can be ussable, mainly for testing (but other purposes too), so this patch adds support for relaxing definition in virsh-edit and makes 'virsh edit <domain>' more usable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - make a special case 'v' for turning off validation
tools/virsh-domain.c | 6 ++++++ tools/virsh-edit.c | 21 +++++++++++++++++++-- tools/virsh.c | 28 +++++++++++++++++++++------- tools/virsh.h | 4 ++-- 4 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2506b89..b4761c6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11265,7 +11265,13 @@ cmdEdit(vshControl *ctl, const vshCmd *cmd) } while (0) #define EDIT_DEFINE \ (dom_edited = vshDomainDefine(ctl->conn, doc_edited, define_flags)) +#define EDIT_RELAX \ + do { \ + define_flags &= ~VIR_DOMAIN_DEFINE_VALIDATE; \ + } while (0); + #include "virsh-edit.c" +#undefine EDIT_RELAX
I did s/undefine/undef/ here, but forgot to squash it in before sending, but my local commit is fixed, don't worry.
On 02/19/2015 11:24 AM, Martin Kletzander wrote:
When editing a domain with 'virsh edit' and failing validation, the usual message pops up:
Failed. Try again? [y,n,f,?]:
Turning of validation can be ussable, mainly for testing (but other
s/of/off s/ussable/useful
purposes too), so this patch adds support for relaxing definition in virsh-edit and makes 'virsh edit <domain>' more usable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - make a special case 'v' for turning off validation
Another choice 'i' "ignore validation" - only because when I see -v I think "verbose" or "version" (just like -h is help) Would it be worth while to add some "history" (eg, stuff from these reviews) to virsh.pod under 'edit' in order to detail the various options that you may be presented with if the edit fails, why you may get them, and perhaps which to choose based on desired results. The code below looks fine to me and I'm not going to object to which letter you chose - I was just presenting options... John
tools/virsh-domain.c | 6 ++++++ tools/virsh-edit.c | 21 +++++++++++++++++++-- tools/virsh.c | 28 +++++++++++++++++++++------- tools/virsh.h | 4 ++-- 4 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2506b89..b4761c6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11265,7 +11265,13 @@ cmdEdit(vshControl *ctl, const vshCmd *cmd) } while (0) #define EDIT_DEFINE \ (dom_edited = vshDomainDefine(ctl->conn, doc_edited, define_flags)) +#define EDIT_RELAX \ + do { \ + define_flags &= ~VIR_DOMAIN_DEFINE_VALIDATE; \ + } while (0); + #include "virsh-edit.c" +#undefine EDIT_RELAX
vshPrint(ctl, _("Domain %s XML configuration edited.\n"), virDomainGetName(dom_edited)); diff --git a/tools/virsh-edit.c b/tools/virsh-edit.c index 41a6d53..2a13a04 100644 --- a/tools/virsh-edit.c +++ b/tools/virsh-edit.c @@ -1,7 +1,7 @@ /* * virsh-edit.c: Implementation of generic virsh *-edit intelligence * - * Copyright (C) 2012 Red Hat, Inc. + * Copyright (C) 2012, 2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -62,6 +62,7 @@ do { char *doc_reread = NULL; const char *msg = NULL; bool edit_success = false; + bool relax_avail = false;
/* Get the XML configuration of the object. */ doc = (EDIT_GET_XML); @@ -74,6 +75,11 @@ do { goto edit_cleanup;
reedit: + +#ifdef EDIT_RELAX + relax_avail = true; +#endif + /* Start the editor. */ if (vshEditFile(ctl, tmp) == -1) goto edit_cleanup; @@ -112,7 +118,7 @@ do { msg = _("Failed.");
if (msg) { - int c = vshAskReedit(ctl, msg); + int c = vshAskReedit(ctl, msg, relax_avail); switch (c) { case 'y': goto reedit; @@ -126,6 +132,17 @@ do { goto edit_cleanup; break;
+#ifdef EDIT_RELAX + case 'v': + if (relax_avail) { + EDIT_RELAX; + relax_avail = false; + goto redefine; + break; + } + /* fall-through */ +#endif + default: vshError(ctl, "%s", msg); break; diff --git a/tools/virsh.c b/tools/virsh.c index aba34ce..464af3d 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1,7 +1,7 @@ /* * virsh.c: a shell to exercise the libvirt API * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -512,13 +512,14 @@ vshPrintRaw(vshControl *ctl, ...) * edited file. * * Returns 'y' if he wants to - * 'f' if he forcibly wants to * 'n' if he doesn't want to + * 'v' if he wants to try defining it again with validation turned off + * 'f' if he forcibly wants to * -1 on error * 0 otherwise */ int -vshAskReedit(vshControl *ctl, const char *msg) +vshAskReedit(vshControl *ctl, const char *msg, bool relax_avail) { int c = -1;
@@ -532,8 +533,9 @@ vshAskReedit(vshControl *ctl, const char *msg)
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,?]:")); + * choices really are limited to just 'y', 'n', 'v', 'f' and '?' */ + vshPrint(ctl, _("\r%s Try again? %s: "), msg, + relax_avail ? "[y,n,v,f,?]" : "[y,n,f,?]"); c = c_tolower(getchar());
if (c == '?') { @@ -541,11 +543,21 @@ vshAskReedit(vshControl *ctl, const char *msg) "", _("y - yes, start editor again"), _("n - no, throw away my changes"), + NULL); + + if (relax_avail) { + vshPrintRaw(ctl, + _("v - turn off validation and try to redefine again"), + NULL); + } + + vshPrintRaw(ctl, _("f - force, try to redefine again"), _("? - print this help"), NULL); continue; - } else if (c == 'y' || c == 'n' || c == 'f') { + } else if (c == 'y' || c == 'n' || c == 'f' || + (relax_avail && c == 'v')) { break; } } @@ -557,7 +569,9 @@ vshAskReedit(vshControl *ctl, const char *msg) } #else /* WIN32 */ int -vshAskReedit(vshControl *ctl, const char *msg ATTRIBUTE_UNUSED) +vshAskReedit(vshControl *ctl, + const char *msg ATTRIBUTE_UNUSED, + bool relax_avail ATTRIBUTE_UNUSED) { vshDebug(ctl, VSH_ERR_WARNING, "%s", _("This function is not " "supported on WIN32 platform")); diff --git a/tools/virsh.h b/tools/virsh.h index 7d5d8a2..df2ea7f 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -1,7 +1,7 @@ /* * virsh.h: a shell to exercise the libvirt API * - * Copyright (C) 2005, 2007-2014 Red Hat, Inc. + * Copyright (C) 2005, 2007-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -359,7 +359,7 @@ char *vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item) char *vshEditWriteToTempFile(vshControl *ctl, const char *doc); int vshEditFile(vshControl *ctl, const char *filename); char *vshEditReadBackFile(vshControl *ctl, const char *filename); -int vshAskReedit(vshControl *ctl, const char *msg); +int vshAskReedit(vshControl *ctl, const char *msg, bool relax_avail); int vshStreamSink(virStreamPtr st, const char *bytes, size_t nbytes, void *opaque); double vshPrettyCapacity(unsigned long long val, const char **unit); -- 2.3.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/19/2015 12:26 PM, John Ferlan wrote:
On 02/19/2015 11:24 AM, Martin Kletzander wrote:
When editing a domain with 'virsh edit' and failing validation, the usual message pops up:
Failed. Try again? [y,n,f,?]:
Turning of validation can be ussable, mainly for testing (but other
s/of/off s/ussable/useful
purposes too), so this patch adds support for relaxing definition in virsh-edit and makes 'virsh edit <domain>' more usable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - make a special case 'v' for turning off validation
Another choice 'i' "ignore validation" - only because when I see -v I think "verbose" or "version" (just like -h is help)
I like the idea of 'i' instead of 'v' as well.
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,?]:")); + * choices really are limited to just 'y', 'n', 'v', 'f' and '?' */ + vshPrint(ctl, _("\r%s Try again? %s: "), msg, + relax_avail ? "[y,n,v,f,?]" : "[y,n,f,?]");
That doesn't translate very well. You must NOT put \r in the string marked for translation ('make dist' will spit out LOTS of these warnings): libvirt.pot:30827: warning: internationalized messages should not contain the '\r' escape sequence Also, ?: can confuse xgettext, which in turn makes the .pot file harder for translators to see. Pre-patch, the .pot file has: #. TRANSLATORS: For now, we aren't using LC_MESSAGES, and the user #. * choices really are limited to just 'y', 'n', 'f' and '?' #: tools/virsh.c:536 msgid "Try again? [y,n,f,?]:" msgstr "" post-patch, the .pot file would have: #. TRANSLATORS: For now, we aren't using LC_MESSAGES, and the user #. * choices really are limited to just 'y', 'n', 'v', 'f' and '?' #: tools/virsh.c:536 #, c-format msgid "\r%s Try again? %s: " msgstr "" but NO mention of a [y,n,...] string, which makes the comment weird. But since the whole point of the comment was that we AREN'T allowing translated option letters (for example, a Spanish user must still type 'y', rather than 's' for "si"), there's no need to translate the options, and therefore no need for the comment. So you should drop the comment, and just use: vshPrint(ctl, "\r%s %s %s: ", msg, _("Try again?"), relax_avail ? "[y,n,v,f,?]" : "[y,n,f,?]"); (with 'i' instead of 'v' if we go that route) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Feb 19, 2015 at 01:16:07PM -0700, Eric Blake wrote:
On 02/19/2015 12:26 PM, John Ferlan wrote:
On 02/19/2015 11:24 AM, Martin Kletzander wrote:
When editing a domain with 'virsh edit' and failing validation, the usual message pops up:
Failed. Try again? [y,n,f,?]:
Turning of validation can be ussable, mainly for testing (but other
s/of/off s/ussable/useful
purposes too), so this patch adds support for relaxing definition in virsh-edit and makes 'virsh edit <domain>' more usable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - make a special case 'v' for turning off validation
Another choice 'i' "ignore validation" - only because when I see -v I think "verbose" or "version" (just like -h is help)
I like the idea of 'i' instead of 'v' as well.
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,?]:")); + * choices really are limited to just 'y', 'n', 'v', 'f' and '?' */ + vshPrint(ctl, _("\r%s Try again? %s: "), msg, + relax_avail ? "[y,n,v,f,?]" : "[y,n,f,?]");
That doesn't translate very well. You must NOT put \r in the string marked for translation ('make dist' will spit out LOTS of these warnings):
libvirt.pot:30827: warning: internationalized messages should not contain the '\r' escape sequence
Also, ?: can confuse xgettext, which in turn makes the .pot file harder for translators to see. Pre-patch, the .pot file has:
#. TRANSLATORS: For now, we aren't using LC_MESSAGES, and the user #. * choices really are limited to just 'y', 'n', 'f' and '?' #: tools/virsh.c:536 msgid "Try again? [y,n,f,?]:" msgstr ""
post-patch, the .pot file would have:
#. TRANSLATORS: For now, we aren't using LC_MESSAGES, and the user #. * choices really are limited to just 'y', 'n', 'v', 'f' and '?' #: tools/virsh.c:536 #, c-format msgid "\r%s Try again? %s: " msgstr ""
but NO mention of a [y,n,...] string, which makes the comment weird. But since the whole point of the comment was that we AREN'T allowing translated option letters (for example, a Spanish user must still type 'y', rather than 's' for "si"), there's no need to translate the options, and therefore no need for the comment. So you should drop the comment, and just use:
vshPrint(ctl, "\r%s %s %s: ", msg, _("Try again?"), relax_avail ? "[y,n,v,f,?]" : "[y,n,f,?]");
(with 'i' instead of 'v' if we go that route)
I had no idea that comments before marked translations go to the .pot file, that's great. What I wanted to achieve is pretty much what you wrote and for the same reason -- not allowing translators to change the characters. "\r%s %s %s: " is definitely way better, I should've thought about making translator lives easier ;) And according to others, 'i' is more likeable, or at least it looks like it. v3 coming up.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/19/2015 02:26 PM, John Ferlan wrote:
On 02/19/2015 11:24 AM, Martin Kletzander wrote:
When editing a domain with 'virsh edit' and failing validation, the usual message pops up:
Failed. Try again? [y,n,f,?]:
Turning of validation can be ussable, mainly for testing (but other s/of/off s/ussable/useful
purposes too), so this patch adds support for relaxing definition in virsh-edit and makes 'virsh edit <domain>' more usable.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- v2: - make a special case 'v' for turning off validation Another choice 'i' "ignore validation" - only because when I see -v I think "verbose" or "version" (just like -h is help)
I think I like "i" for "ignore validation errors" rather than "v", for exactly the same reason - v almost always means "verbose" or "version".
participants (4)
-
Eric Blake -
John Ferlan -
Laine Stump -
Martin Kletzander