[libvirt] [PATCH] virsh: Improve editing
When users (pool-/net-)edit and they make a mistake, temporary file and thus all changes are gone. Better way is to let them decide if they want to get back to edit and correct what's wrong. However, this is suitable only in interactive mode. --- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 40 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index fbeb7c8..0dccc17 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -338,6 +338,7 @@ static const char *vshDomainStateToString(int state); static const char *vshDomainVcpuStateToString(int state); static bool vshConnectionUsability(vshControl *ctl, virConnectPtr conn); +static char * vshReadline (vshControl *ctl, const char *prompt); static char *editWriteToTempFile (vshControl *ctl, const char *doc); static int editFile (vshControl *ctl, const char *filename); static char *editReadBackFile (vshControl *ctl, const char *filename); @@ -10099,6 +10100,7 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) char *doc_edited = NULL; char *doc_reread = NULL; int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE; + char *response; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -10116,6 +10118,7 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) tmp = editWriteToTempFile (ctl, doc); if (!tmp) goto cleanup; +again: /* Start the editor. */ if (editFile (ctl, tmp) == -1) goto cleanup; @@ -10148,8 +10151,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; + if (!dom) { + if (ctl->imode) { + virshReportError(ctl); + response = vshReadline(ctl, "Do you want to edit again [Y/n] "); + while (true) { + char *c = response; + while (c && *c) { + *c = tolower(*c); + c++; + } + + if (STREQ(response, "") || STREQ(response, "y") || + STREQ(response, "yes")) { + dom = vshCommandOptDomain (ctl, cmd, NULL); + if (!dom) + goto cleanup; + goto again; + } else if (!response || STREQ(response, "n") || + STREQ(response, "no")) { + /* Ok, this is a little hack. Command did not succeed, + * but the error was already reported. So we do not + * want it to get reported twice */ + ret = true; + goto cleanup; + } else { + char *prompt; + if (virAsprintf(&prompt, _("Sorry, response '%s' not " + "understood. [Y/n] "), response) < 0) + goto cleanup; + response = vshReadline(ctl, prompt); + VIR_FREE(prompt); + } + } + } else { + goto cleanup; + } + } vshPrint (ctl, _("Domain %s XML configuration edited.\n"), virDomainGetName(dom)); -- 1.7.5.rc3
On Fri, May 13, 2011 at 12:34:25 +0200, Michal Privoznik wrote:
When users (pool-/net-)edit and they make a mistake, temporary file and thus all changes are gone. Better way is to let them decide if they want to get back to edit and correct what's wrong. However, this is suitable only in interactive mode. --- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 40 insertions(+), 2 deletions(-)
I like the idea (not sure if that's because I suggested it :-P) but, I have a few comments. The patch doesn't touch iface-edit, which is not generated from cmdEdit, though the right fix might turn out to be generating cmdIfaceEdit together with cmdPoolEdit and cmdNetworkEdit. And IMHO *edit commands are always interactive even when not called in virsh interactive mode so I'd prefer this behaviour to be unconditional. I can't imagine anyone sane to call virsh edit from a script, it's much easier to just call virsh dumpxml, change the xml and virsh define it back. Jirka
On Fri, May 13, 2011 at 02:57:08PM +0200, Jiri Denemark wrote:
On Fri, May 13, 2011 at 12:34:25 +0200, Michal Privoznik wrote:
When users (pool-/net-)edit and they make a mistake, temporary file and thus all changes are gone. Better way is to let them decide if they want to get back to edit and correct what's wrong. However, this is suitable only in interactive mode. --- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 40 insertions(+), 2 deletions(-)
I like the idea (not sure if that's because I suggested it :-P) but, I have a few comments. The patch doesn't touch iface-edit, which is not generated from cmdEdit, though the right fix might turn out to be generating cmdIfaceEdit together with cmdPoolEdit and cmdNetworkEdit.
And IMHO *edit commands are always interactive even when not called in virsh interactive mode so I'd prefer this behaviour to be unconditional. I can't imagine anyone sane to call virsh edit from a script, it's much easier to just call virsh dumpxml, change the xml and virsh define it back.
There are two common problems with virsh edit & friends - Invalid XML syntax, causes error report & lost changes - User add unsupported/unknown XML attributes/elements which are silently discarded by libvirt This patch only fixes the first problem. It would be nice to fix the second two, by running the XML through the RNG schema validator. Rather than do this in virsh though, I'd add some flags to the virXXXXDefine/Create APIs, eg VIR_DOMAIN_XML_VALIDATE virsh can set this flag by default, and if the XML fails validation, it could prompt the user, asking if they want to proceed anyway (in which case recall the same API but without the validate flag set), or re-edit the XML 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 Fri, May 13, 2011 at 02:42:29PM +0100, Daniel P. Berrange wrote:
There are two common problems with virsh edit & friends
- Invalid XML syntax, causes error report & lost changes - User add unsupported/unknown XML attributes/elements which are silently discarded by libvirt
This patch only fixes the first problem. It would be nice to fix the second two, by running the XML through the RNG schema validator.
Rather than do this in virsh though, I'd add some flags to the virXXXXDefine/Create APIs, eg
VIR_DOMAIN_XML_VALIDATE
virsh can set this flag by default, and if the XML fails validation, it could prompt the user, asking if they want to proceed anyway (in which case recall the same API but without the validate flag set), or re-edit the XML
Hum, yes I agree with the option of validating on define of APIs the only problem is that we tend to have holes in the RNG, but since that would be optional I think that's okay, this would hopefully help finding the mismatches between the RNG and the C parsing code. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 05/16/2011 01:12 AM, Daniel Veillard wrote:
On Fri, May 13, 2011 at 02:42:29PM +0100, Daniel P. Berrange wrote:
There are two common problems with virsh edit & friends
- Invalid XML syntax, causes error report & lost changes - User add unsupported/unknown XML attributes/elements which are silently discarded by libvirt
This patch only fixes the first problem. It would be nice to fix the second two, by running the XML through the RNG schema validator.
Rather than do this in virsh though, I'd add some flags to the virXXXXDefine/Create APIs, eg
VIR_DOMAIN_XML_VALIDATE
virsh can set this flag by default, and if the XML fails validation, it could prompt the user, asking if they want to proceed anyway (in which case recall the same API but without the validate flag set), or re-edit the XML
Hum, yes I agree with the option of validating on define of APIs the only problem is that we tend to have holes in the RNG, but since that would be optional I think that's okay, this would hopefully help finding the mismatches between the RNG and the C parsing code.
There's also the idea of doing a round trip parse -> dumpxml -> compare; but that also has problems where dumpxml sometimes rearranges elements or populates backwards-compatibility additions that were not present in the original. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, May 16, 2011 at 02:59:28PM -0600, Eric Blake wrote:
On 05/16/2011 01:12 AM, Daniel Veillard wrote:
On Fri, May 13, 2011 at 02:42:29PM +0100, Daniel P. Berrange wrote:
There are two common problems with virsh edit & friends
- Invalid XML syntax, causes error report & lost changes - User add unsupported/unknown XML attributes/elements which are silently discarded by libvirt
This patch only fixes the first problem. It would be nice to fix the second two, by running the XML through the RNG schema validator.
Rather than do this in virsh though, I'd add some flags to the virXXXXDefine/Create APIs, eg
VIR_DOMAIN_XML_VALIDATE
virsh can set this flag by default, and if the XML fails validation, it could prompt the user, asking if they want to proceed anyway (in which case recall the same API but without the validate flag set), or re-edit the XML
Hum, yes I agree with the option of validating on define of APIs the only problem is that we tend to have holes in the RNG, but since that would be optional I think that's okay, this would hopefully help finding the mismatches between the RNG and the C parsing code.
There's also the idea of doing a round trip parse -> dumpxml -> compare; but that also has problems where dumpxml sometimes rearranges elements or populates backwards-compatibility additions that were not present in the original.
Yeah that can't fly, even for an XML/Markup editor preserving this kind of non-strucural information is nearly impossible, since we even discard the tree in libvirt after parsing we really can't hope to do anything like this. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 05/13/2011 08:57 AM, Jiri Denemark wrote:
On Fri, May 13, 2011 at 12:34:25 +0200, Michal Privoznik wrote:
When users (pool-/net-)edit and they make a mistake, temporary file and thus all changes are gone. Better way is to let them decide if they want to get back to edit and correct what's wrong. However, this is suitable only in interactive mode. --- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 40 insertions(+), 2 deletions(-)
I like the idea (not sure if that's because I suggested it :-P) but, I have a few comments. The patch doesn't touch iface-edit, which is not generated from cmdEdit, though the right fix might turn out to be generating cmdIfaceEdit together with cmdPoolEdit and cmdNetworkEdit.
I think we should just drop the 'edit' generation. It's a one-off that doesn't fit with the rest of the virsh code. I'm sure most of the shared functionality could be broken out into helper functions. Either that or embrace it and generate more virsh commands :) - Cole
On 05/13/2011 10:23 AM, Cole Robinson wrote:
On 05/13/2011 08:57 AM, Jiri Denemark wrote:
On Fri, May 13, 2011 at 12:34:25 +0200, Michal Privoznik wrote:
When users (pool-/net-)edit and they make a mistake, temporary file and thus all changes are gone. Better way is to let them decide if they want to get back to edit and correct what's wrong. However, this is suitable only in interactive mode. --- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 40 insertions(+), 2 deletions(-) I like the idea (not sure if that's because I suggested it :-P) but, I have a few comments. The patch doesn't touch iface-edit, which is not generated from cmdEdit, though the right fix might turn out to be generating cmdIfaceEdit together with cmdPoolEdit and cmdNetworkEdit.
I think we should just drop the 'edit' generation. It's a one-off that doesn't fit with the rest of the virsh code. I'm sure most of the shared functionality could be broken out into helper functions.
Ah, yes. I'd forgotten all about that. Looking back at the code to remember why I didn't make cmdInterfaceEdit another generated file like cmdPoolEdit and cmdNetEdit, I come up with the following: I didn't like the the idea of generated code that was creating by scraping a piece out of an existing C file. It all felt very hack-ey, and I didn't want to perpetuate that. (I should have gone back later and done something in one direction or the other to eliminate this hackiness, but as I said, I immediately forgot all about it until I was reminded now, nearly two years later). I would probably have no complaint about it if the original source used for the generation was not just something dredged out of an existing C file, but was instead kept separately, and was truly a template rather than an actual function that was also compiled as-is (ie cmdEdit was also generated, and the strings being replaced were clearly marked in the original as such). However, I think I prefer Cole's idea of putting the shared functionality into helper functions, or possibly a single function that took pointers to a few object-specific functions as arguments, so it could be called like this: ret = cmdEdit(ctl, cmd, vshCommandOptInterface, virInterfaceGetXMLDesc, virInterfaceGetName, virInterfaceDefineXML, virInterfaceFree, VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE; ); (there again the (easily solved) problem is that all of the vir*DefineXML take a flags argument, except for virDomainDefineXML. And of course there's the problem that the functions for each object type do not have exactly the same prototype, since the object being pointed to is different in each case. But this is C, so pesky type-checking problems can always be eliminated :-)
On 05/13/2011 06:57 AM, Jiri Denemark wrote:
On Fri, May 13, 2011 at 12:34:25 +0200, Michal Privoznik wrote:
When users (pool-/net-)edit and they make a mistake, temporary file and thus all changes are gone. Better way is to let them decide if they want to get back to edit and correct what's wrong. However, this is suitable only in interactive mode. --- tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 40 insertions(+), 2 deletions(-)
I like the idea (not sure if that's because I suggested it :-P) but, I have a few comments. The patch doesn't touch iface-edit, which is not generated from cmdEdit, though the right fix might turn out to be generating cmdIfaceEdit together with cmdPoolEdit and cmdNetworkEdit.
And IMHO *edit commands are always interactive even when not called in virsh interactive mode so I'd prefer this behaviour to be unconditional.
I don't know - I could see a semi-reasonable chance of scripting 'EDITOR=sed virsh edit < some_sed_script'.
I can't imagine anyone sane to call virsh edit from a script, it's much easier to just call virsh dumpxml, change the xml and virsh define it back.
But you have a point here. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (7)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jiri Denemark -
Laine Stump -
Michal Privoznik