[libvirt] [PATCH] virsh: Allow users to reedit rejected XML
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes. --- tools/console.c | 40 +++++++++++++++++++++++++--------------- tools/console.h | 1 + tools/virsh.c | 41 +++++++++++++++++++++++++++++++++++------ 3 files changed, 61 insertions(+), 21 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) { + struct termios rawattr; + + if (tcgetattr(STDIN_FILENO, ttyattr) < 0) { + if (report_errors) + VIR_ERROR(_("unable to get tty attributes: %s"), + strerror(errno)); + return -1; + } + + rawattr = *ttyattr; + cfmakeraw(&rawattr); + + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { + if (report_errors) + VIR_ERROR(_("unable to set tty attributes: %s"), + strerror(errno)); + return -1; + } + + return 0; +} + int vshRunConsole(virDomainPtr dom, const char *dev_name, const char *escape_seq, unsigned int flags) { int ret = -1; - struct termios ttyattr, rawattr; + struct termios ttyattr; void (*old_sigquit)(int); void (*old_sigterm)(int); void (*old_sigint)(int); @@ -317,21 +340,8 @@ int vshRunConsole(virDomainPtr dom, result in it being echoed back already), and also ensure Ctrl-C, etc is blocked, and misc other bits */ - if (tcgetattr(STDIN_FILENO, &ttyattr) < 0) { - VIR_ERROR(_("unable to get tty attributes: %s"), - strerror(errno)); - return -1; - } - - rawattr = ttyattr; - cfmakeraw(&rawattr); - - if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { - VIR_ERROR(_("unable to set tty attributes: %s"), - strerror(errno)); + if (vshMakeStdinRaw(&ttyattr, true) < 0) goto resettty; - } - /* Trap all common signals so that we can safely restore the original terminal settings on STDIN before the diff --git a/tools/console.h b/tools/console.h index 2b5440c..97c97cd 100644 --- a/tools/console.h +++ b/tools/console.h @@ -30,6 +30,7 @@ int vshRunConsole(virDomainPtr dom, const char *escape_seq, unsigned int flags); +int vshMakeStdinRaw(struct termios *ttyattr, bool report_errors); # endif /* !WIN32 */ #endif /* __VIR_CONSOLE_H__ */ diff --git a/tools/virsh.c b/tools/virsh.c index dd9292a..3537e2e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -33,6 +33,7 @@ #include <signal.h> #include <poll.h> #include <strings.h> +#include <termios.h> #include <libxml/parser.h> #include <libxml/tree.h> @@ -15806,11 +15807,14 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) { bool ret = false; virDomainPtr dom = NULL; + virDomainPtr dom_edited = NULL; char *tmp = NULL; char *doc = NULL; char *doc_edited = NULL; char *doc_reread = NULL; unsigned int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE; + int c = 0; + struct termios ttyattr; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -15828,6 +15832,7 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) tmp = editWriteToTempFile (ctl, doc); if (!tmp) goto cleanup; +reedit: /* Start the editor. */ if (editFile (ctl, tmp) == -1) goto cleanup; @@ -15858,19 +15863,43 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) } /* Everything checks out, so redefine the domain. */ - virDomainFree (dom); - dom = virDomainDefineXML(ctl->conn, doc_edited); - if (!dom) - goto cleanup; + dom_edited = virDomainDefineXML(ctl->conn, doc_edited); + if (!dom_edited) { + /* Redefine failed. If we are in interactive mode ask user + * if he wants to re-edit the XML. */ + if (!ctl->imode || + vshMakeStdinRaw(&ttyattr, false) < 0) + goto cleanup; + + virshReportError(ctl); + + while (true) { + vshPrint(ctl, "\rFailed. Try again? (y/Y/n/N) [Y]:"); + c = getchar(); + c = c_toupper(c); + if (c == '\n' || c == '\r' || c == 'Y' || c == 'N') + break; + } + + tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); + + if (c == 'N') + goto cleanup; + + vshPrint(ctl, "\r\n"); + goto reedit; + } vshPrint (ctl, _("Domain %s XML configuration edited.\n"), virDomainGetName(dom)); ret = true; - cleanup: +cleanup: if (dom) - virDomainFree (dom); + virDomainFree(dom); + if (dom_edited) + virDomainFree(dom_edited); VIR_FREE(doc); VIR_FREE(doc_edited); -- 1.7.8.5
On Wed, May 09, 2012 at 03:36:26PM +0200, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes. ---
+ dom_edited = virDomainDefineXML(ctl->conn, doc_edited); + if (!dom_edited) { + /* Redefine failed. If we are in interactive mode ask user + * if he wants to re-edit the XML. */ + if (!ctl->imode || + vshMakeStdinRaw(&ttyattr, false) < 0) + goto cleanup;
I don't see why this has to be restricted to interactive mode only. I almost always just run virsh edit foo and there's no reason why we can't prompt to re-edit here too. What you want to check is isatty(STDIN) so you can distinguish batch scripting. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/09/2012 07:52 AM, Daniel P. Berrange wrote:
On Wed, May 09, 2012 at 03:36:26PM +0200, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes. ---
+ dom_edited = virDomainDefineXML(ctl->conn, doc_edited); + if (!dom_edited) { + /* Redefine failed. If we are in interactive mode ask user + * if he wants to re-edit the XML. */ + if (!ctl->imode || + vshMakeStdinRaw(&ttyattr, false) < 0) + goto cleanup;
I don't see why this has to be restricted to interactive mode only. I almost always just run
virsh edit foo
and there's no reason why we can't prompt to re-edit here too.
What you want to check is isatty(STDIN) so you can distinguish batch scripting.
And given that I don't want to bump to a new gnulib version until after 0.9.12, that means we probably should delay an isatty() solution until after this patch is in: https://www.redhat.com/archives/libvir-list/2012-May/msg00402.html -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, May 09, 2012 at 03:36:26PM +0200, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes.
ACK to the design, what a great usability enhancement. Dave
--- tools/console.c | 40 +++++++++++++++++++++++++--------------- tools/console.h | 1 + tools/virsh.c | 41 +++++++++++++++++++++++++++++++++++------ 3 files changed, 61 insertions(+), 21 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) { + struct termios rawattr; + + if (tcgetattr(STDIN_FILENO, ttyattr) < 0) { + if (report_errors) + VIR_ERROR(_("unable to get tty attributes: %s"), + strerror(errno)); + return -1; + } + + rawattr = *ttyattr; + cfmakeraw(&rawattr); + + if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { + if (report_errors) + VIR_ERROR(_("unable to set tty attributes: %s"), + strerror(errno)); + return -1; + } + + return 0; +} + int vshRunConsole(virDomainPtr dom, const char *dev_name, const char *escape_seq, unsigned int flags) { int ret = -1; - struct termios ttyattr, rawattr; + struct termios ttyattr; void (*old_sigquit)(int); void (*old_sigterm)(int); void (*old_sigint)(int); @@ -317,21 +340,8 @@ int vshRunConsole(virDomainPtr dom, result in it being echoed back already), and also ensure Ctrl-C, etc is blocked, and misc other bits */ - if (tcgetattr(STDIN_FILENO, &ttyattr) < 0) { - VIR_ERROR(_("unable to get tty attributes: %s"), - strerror(errno)); - return -1; - } - - rawattr = ttyattr; - cfmakeraw(&rawattr); - - if (tcsetattr(STDIN_FILENO, TCSAFLUSH, &rawattr) < 0) { - VIR_ERROR(_("unable to set tty attributes: %s"), - strerror(errno)); + if (vshMakeStdinRaw(&ttyattr, true) < 0) goto resettty; - } -
/* Trap all common signals so that we can safely restore the original terminal settings on STDIN before the diff --git a/tools/console.h b/tools/console.h index 2b5440c..97c97cd 100644 --- a/tools/console.h +++ b/tools/console.h @@ -30,6 +30,7 @@ int vshRunConsole(virDomainPtr dom, const char *escape_seq, unsigned int flags);
+int vshMakeStdinRaw(struct termios *ttyattr, bool report_errors); # endif /* !WIN32 */
#endif /* __VIR_CONSOLE_H__ */ diff --git a/tools/virsh.c b/tools/virsh.c index dd9292a..3537e2e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -33,6 +33,7 @@ #include <signal.h> #include <poll.h> #include <strings.h> +#include <termios.h>
#include <libxml/parser.h> #include <libxml/tree.h> @@ -15806,11 +15807,14 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) { bool ret = false; virDomainPtr dom = NULL; + virDomainPtr dom_edited = NULL; char *tmp = NULL; char *doc = NULL; char *doc_edited = NULL; char *doc_reread = NULL; unsigned int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE; + int c = 0; + struct termios ttyattr;
if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -15828,6 +15832,7 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) tmp = editWriteToTempFile (ctl, doc); if (!tmp) goto cleanup;
+reedit: /* Start the editor. */ if (editFile (ctl, tmp) == -1) goto cleanup;
@@ -15858,19 +15863,43 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) }
/* Everything checks out, so redefine the domain. */ - virDomainFree (dom); - dom = virDomainDefineXML(ctl->conn, doc_edited); - if (!dom) - goto cleanup; + dom_edited = virDomainDefineXML(ctl->conn, doc_edited); + if (!dom_edited) { + /* Redefine failed. If we are in interactive mode ask user + * if he wants to re-edit the XML. */ + if (!ctl->imode || + vshMakeStdinRaw(&ttyattr, false) < 0) + goto cleanup; + + virshReportError(ctl); + + while (true) { + vshPrint(ctl, "\rFailed. Try again? (y/Y/n/N) [Y]:"); + c = getchar(); + c = c_toupper(c); + if (c == '\n' || c == '\r' || c == 'Y' || c == 'N') + break; + } + + tcsetattr(STDIN_FILENO, TCSAFLUSH, &ttyattr); + + if (c == 'N') + goto cleanup; + + vshPrint(ctl, "\r\n"); + goto reedit; + }
vshPrint (ctl, _("Domain %s XML configuration edited.\n"), virDomainGetName(dom));
ret = true;
- cleanup: +cleanup: if (dom) - virDomainFree (dom); + virDomainFree(dom); + if (dom_edited) + virDomainFree(dom_edited);
VIR_FREE(doc); VIR_FREE(doc_edited); -- 1.7.8.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/09/2012 09:36 AM, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes.
This all reminds me that I so much disliked the idea of creating a .c file by running sed against virsh.c and then #include'ing that .c file into virsh.c that I instead made a separate function for cmdInterfaceEdit rather than adding to it. I had thought that someone refactored that long ago so that the main bit of code was a helper function called by all (thus removing the sed trickery), but apparently we only talked about it. (*adds a line to todo list*)
On 10.05.2012 19:10, Laine Stump wrote:
On 05/09/2012 09:36 AM, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes.
This all reminds me that I so much disliked the idea of creating a .c file by running sed against virsh.c and then #include'ing that .c file into virsh.c that I instead made a separate function for cmdInterfaceEdit rather than adding to it. I had thought that someone refactored that long ago so that the main bit of code was a helper function called by all (thus removing the sed trickery), but apparently we only talked about it. (*adds a line to todo list*)
Heh, thanks for pointing that out. I've completely forgotten about iface-edit. So my patch is incomplete. :( On the other hand, seems like sed script is doing its work well. I've taken look at generated code and it was good. Even when I've introduced this dom_edited variable, it was renamed to network_edited and pool_edited respectively in those generated functions. So honestly, unless we are doing some different magic in cmdInterfaceEdit (quick look doesn't say so) I think we can believe the sed script and make cmdInterfaceEdit being generated as well. The advantage is - if somebody (this time it's me) introduce any feature, fix a bug, all other *-edit commands benefit from it immediately. Michal
On 05/11/2012 02:07 AM, Michal Privoznik wrote:
On 10.05.2012 19:10, Laine Stump wrote:
On 05/09/2012 09:36 AM, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes.
This all reminds me that I so much disliked the idea of creating a .c file by running sed against virsh.c and then #include'ing that .c file into virsh.c that I instead made a separate function for cmdInterfaceEdit rather than adding to it. I had thought that someone refactored that long ago so that the main bit of code was a helper function called by all (thus removing the sed trickery), but apparently we only talked about it. (*adds a line to todo list*)
Heh, thanks for pointing that out. I've completely forgotten about iface-edit. So my patch is incomplete. :( On the other hand, seems like sed script is doing its work well. I've taken look at generated code and it was good. Even when I've introduced this dom_edited variable, it was renamed to network_edited and pool_edited respectively in those generated functions. So honestly, unless we are doing some different magic in cmdInterfaceEdit (quick look doesn't say so) I think we can believe the sed script and make cmdInterfaceEdit being generated as well. The advantage is - if somebody (this time it's me) introduce any feature, fix a bug, all other *-edit commands benefit from it immediately.
Still, I can't help but wonder if we can refactor this into a generic helper function that takes a callback pointer for the specific API to call, rather than a sed script. In addition to iface-edit not using the sed script, we also have save-image-edit, nwfilter-edit, and snapshot-edit that all need fixing. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, May 11, 2012 at 06:15:29AM -0600, Eric Blake wrote:
On 05/11/2012 02:07 AM, Michal Privoznik wrote:
On 10.05.2012 19:10, Laine Stump wrote:
On 05/09/2012 09:36 AM, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes.
This all reminds me that I so much disliked the idea of creating a .c file by running sed against virsh.c and then #include'ing that .c file into virsh.c that I instead made a separate function for cmdInterfaceEdit rather than adding to it. I had thought that someone refactored that long ago so that the main bit of code was a helper function called by all (thus removing the sed trickery), but apparently we only talked about it. (*adds a line to todo list*)
Heh, thanks for pointing that out. I've completely forgotten about iface-edit. So my patch is incomplete. :( On the other hand, seems like sed script is doing its work well. I've taken look at generated code and it was good. Even when I've introduced this dom_edited variable, it was renamed to network_edited and pool_edited respectively in those generated functions. So honestly, unless we are doing some different magic in cmdInterfaceEdit (quick look doesn't say so) I think we can believe the sed script and make cmdInterfaceEdit being generated as well. The advantage is - if somebody (this time it's me) introduce any feature, fix a bug, all other *-edit commands benefit from it immediately.
Still, I can't help but wonder if we can refactor this into a generic helper function that takes a callback pointer for the specific API to call, rather than a sed script.
In addition to iface-edit not using the sed script, we also have save-image-edit, nwfilter-edit, and snapshot-edit that all need fixing.
Or just #define the block of code for editing the XML and pass in the separate API names / types as macro parameters I'd love to see the back of the sed script. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/11/2012 04:07 AM, Michal Privoznik wrote:
On 05/09/2012 09:36 AM, Michal Privoznik wrote:
If users {net-,pool-,}edit but make a mistake in XML all changes are permanently lost. However, if virsh is running in interactive mode we can as user if he wants to re-edit the file and correct the mistakes. This all reminds me that I so much disliked the idea of creating a .c file by running sed against virsh.c and then #include'ing that .c file into virsh.c that I instead made a separate function for cmdInterfaceEdit rather than adding to it. I had thought that someone refactored that long ago so that the main bit of code was a helper function called by all (thus removing the sed trickery), but apparently we only talked about it. (*adds a line to todo list*) Heh, thanks for pointing that out. I've completely forgotten about iface-edit. So my patch is incomplete. :( On the other hand, seems like sed script is doing its work well. I've taken look at generated code and it was good. Even when I've introduced
On 10.05.2012 19:10, Laine Stump wrote: this dom_edited variable, it was renamed to network_edited and pool_edited respectively in those generated functions. So honestly, unless we are doing some different magic in cmdInterfaceEdit (quick look doesn't say so) I think we can believe the sed script and make cmdInterfaceEdit being generated as well. The advantage is - if somebody (this time it's me) introduce any feature, fix a bug, all other *-edit commands benefit from it immediately.
I agree that the code should only be there once, but rather than creating all the other versions with a strange sed script in the makefile, I think the correct solution is the more usual/accepted method of making a generic helper function that parameterizes everything that is different between the different types of edit, with a short function for each version that calls this helper function. (I have this bad feeling that I once promised to do that, but then forgot and never made good on my promise :-/) (or alternately, doing it with a macro as Dan suggests. That makes for a pretty long macro, though :-)
On Fri, May 11, 2012 at 10:28:52AM -0400, Laine Stump wrote:
I agree that the code should only be there once, but rather than creating all the other versions with a strange sed script in the makefile, I think the correct solution is the more usual/accepted method of making a generic helper function that parameterizes everything that is different between the different types of edit, with a short function for each version that calls this helper function. (I have this bad feeling that I once promised to do that, but then forgot and never made good on my promise :-/)
(or alternately, doing it with a macro as Dan suggests. That makes for a pretty long macro, though :-)
Not neccessarily. You can have the "edit" function in a separate file virsh-edit.c, which you then #include several times #define EDIT_TYPE virDomainPtr #define EDIT_API virDomainDefine #include "virsh-edit.c" #define EDIT_TYPE virNetworkPtr #define EDIT_API virNetworkDefine #include "virsh-edit.c" etc Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (5)
-
Daniel P. Berrange -
Dave Allan -
Eric Blake -
Laine Stump -
Michal Privoznik