[libvirt] [PATCH 0/2] Make virsh *edit more bearable
If there's an error in XML, all changes made by user are lost. Users (read /me) tends to get angry with this. Allow them to get back and re-edit. Michal Privoznik (2): virsh: Switch from generated cmd*Edit commands to nongenerated virsh: Allow users to reedit rejected XML cfg.mk | 4 +- po/POTFILES.in | 1 + tools/Makefile.am | 36 +---- tools/console.c | 40 +++-- tools/console.h | 2 + tools/virsh-edit.c | 75 +++++++++ tools/virsh.c | 439 ++++++++++++++++++++-------------------------------- 7 files changed, 277 insertions(+), 320 deletions(-) create mode 100644 tools/virsh-edit.c -- 1.7.8.5
Currently, we either generate some cmd*Edit commands (cmdPoolEdit and cmdNetworkEdit) via sed script or copy the body of cmdEdit (e.g. cmdInterfaceEdit, cmdNWFilterEdit, etc.). This fact makes it harder to implement any new feature to our editing system. Therefore switch to new implementation - define macros to: - dump XML (EDIT_GET_XML) - take an action if XML wasn't changed, usually just vshPrint() (EDIT_NOT_CHANGED) - define new object (EDIT_DEFINE) - the edited XML is in @doc_edited and #include "virsh-edit.c" --- cfg.mk | 4 +- po/POTFILES.in | 1 + tools/Makefile.am | 36 +----- tools/virsh-edit.c | 69 +++++++++ tools/virsh.c | 394 +++++++++++++++++----------------------------------- 5 files changed, 199 insertions(+), 305 deletions(-) create mode 100644 tools/virsh-edit.c diff --git a/cfg.mk b/cfg.mk index 07fb7b2..06a572f 100644 --- a/cfg.mk +++ b/cfg.mk @@ -820,9 +820,9 @@ exclude_file_name_regexp--sc_prohibit_xmlURI = ^src/util/viruri\.c$$ exclude_file_name_regexp--sc_prohibit_return_as_function = \.py$$ -exclude_file_name_regexp--sc_require_config_h = ^examples/ +exclude_file_name_regexp--sc_require_config_h = ^(examples/|tools/virsh-edit.c$$) -exclude_file_name_regexp--sc_require_config_h_first = ^examples/ +exclude_file_name_regexp--sc_require_config_h_first = ^(examples/|tools/virsh-edit.c$$) exclude_file_name_regexp--sc_trailing_blank = \ (/qemuhelpdata/|\.(fig|gif|ico|png)$$) diff --git a/po/POTFILES.in b/po/POTFILES.in index cfa9d44..03fb03f 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -169,6 +169,7 @@ src/xenxs/xen_xm.c tools/console.c tools/libvirt-guests.init.sh tools/virsh.c +tools/virsh-edit.c tools/virt-host-validate-common.c tools/virt-host-validate-lxc.c tools/virt-host-validate-qemu.c diff --git a/tools/Makefile.am b/tools/Makefile.am index c0c907e..7b77595 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -111,41 +111,7 @@ virsh_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(LIBXML_CFLAGS) \ $(READLINE_CFLAGS) -BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c - -virsh-net-edit.c: virsh.c Makefile.am - $(AM_V_GEN)rm -f $@-tmp && \ - echo '/* Automatically generated from: $^ */' > $@-tmp && \ - echo 'static bool' >> $@-tmp && \ - awk '/^cmdEdit/, /^}/' $< \ - | sed -e 's/domain/network/g' \ - -e 's/Domain/Network/g' \ - -e 's/cmdEdit/cmdNetworkEdit/g' \ - -e 's/dom/network/g' \ - -e 's/int flags.*/int flags = 0;/g' \ - >> $@-tmp && \ - chmod a-w $@-tmp && \ - rm -f $@ && \ - mv $@-tmp $@ - -virsh-pool-edit.c: virsh.c Makefile.am - $(AM_V_GEN)rm -f $@-tmp && \ - echo '/* Automatically generated from: $^ */' > $@-tmp && \ - echo 'static bool' >> $@-tmp && \ - awk '/^cmdEdit/, /^}/' $< \ - | sed -e 's/domain/pool/g' \ - -e 's/vshCommandOptDomain/vshCommandOptPool/g' \ - -e 's/Domain %s/Pool %s/g' \ - -e 's/(ctl, cmd, NULL);/(ctl, cmd, "pool", NULL);/' \ - -e 's/Domain/StoragePool/g' \ - -e 's/cmdEdit/cmdPoolEdit/g' \ - -e 's/\(virStoragePoolDefineXML.*\));/\1, 0);/' \ - -e 's/dom/pool/g' \ - -e 's/int flags.*/int flags = 0;/g' \ - >> $@-tmp && \ - chmod a-w $@-tmp && \ - rm -f $@ && \ - mv $@-tmp $@ +BUILT_SOURCES = virsh-edit.c if WITH_WIN_ICON diff --git a/tools/virsh-edit.c b/tools/virsh-edit.c new file mode 100644 index 0000000..060dc14 --- /dev/null +++ b/tools/virsh-edit.c @@ -0,0 +1,69 @@ +do { + char *tmp = NULL; + char *doc = NULL; + char *doc_edited = NULL; + char *doc_reread = NULL; + + /* Get the XML configuration of the domain. */ + doc = EDIT_GET_XML; + if (!doc) + goto edit_cleanup; + + /* Create and open the temporary file. */ + tmp = editWriteToTempFile (ctl, doc); + if (!tmp) + goto edit_cleanup; + + /* Start the editor. */ + if (editFile (ctl, tmp) == -1) + goto edit_cleanup; + + /* Read back the edited file. */ + doc_edited = editReadBackFile (ctl, tmp); + if (!doc_edited) + goto edit_cleanup; + + /* Compare original XML with edited. Has it changed at all? */ + if (STREQ (doc, doc_edited)) { + EDIT_NOT_CHANGED; + ret = true; + goto edit_cleanup; + } + + /* Now re-read the domain XML. Did someone else change it while + * it was being edited? This also catches problems such as us + * losing a connection or the domain going away. + */ + doc_reread = EDIT_GET_XML; + if (!doc_reread) + goto edit_cleanup; + + if (STRNEQ (doc, doc_reread)) { + vshError(ctl, "%s", _("ERROR: the XML configuration " + "was changed by another user")); + goto edit_cleanup; + } + + /* Everything checks out, so redefine the domain. */ + if (!(EDIT_DEFINE)) + goto edit_cleanup; + + goto edit_continue; + +edit_cleanup: + VIR_FREE(doc); + VIR_FREE(doc_edited); + VIR_FREE(doc_reread); + if (tmp) { + unlink (tmp); + VIR_FREE(tmp); + } + goto cleanup; + +} while (0); + +edit_continue: + +#undef EDIT_GET_XML +#undef EDIT_NOT_CHANGED +#undef EDIT_DEFINE diff --git a/tools/virsh.c b/tools/virsh.c index 08b3854..fb8ede4 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3422,9 +3422,6 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) { const char *file = NULL; bool ret = false; - char *tmp = NULL; - char *doc = NULL; - char *doc_edited = NULL; unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE; unsigned int define_flags = 0; @@ -3448,51 +3445,18 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "file", &file) <= 0) return false; - /* Get the XML configuration of the saved image. */ - doc = virDomainSaveImageGetXMLDesc(ctl->conn, file, getxml_flags); - if (!doc) - goto cleanup; - - /* Create and open the temporary file. */ - tmp = editWriteToTempFile(ctl, doc); - if (!tmp) - goto cleanup; - - /* Start the editor. */ - if (editFile(ctl, tmp) == -1) - goto cleanup; - - /* Read back the edited file. */ - doc_edited = editReadBackFile(ctl, tmp); - if (!doc_edited) - goto cleanup; - - /* Compare original XML with edited. Short-circuit if it did not - * change, and we do not have any flags. */ - if (STREQ(doc, doc_edited) && !define_flags) { - vshPrint(ctl, _("Saved image %s XML configuration not changed.\n"), - file); - ret = true; - goto cleanup; - } - - /* Everything checks out, so redefine the xml. */ - if (virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, - define_flags) < 0) { - vshError(ctl, _("Failed to update %s"), file); - goto cleanup; - } + #define EDIT_GET_XML \ + virDomainSaveImageGetXMLDesc(ctl->conn, file, getxml_flags) + #define EDIT_NOT_CHANGED \ + vshPrint(ctl, _("Saved image %s XML configuration not changed.\n"), file) + #define EDIT_DEFINE \ + virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, define_flags) + #include "virsh-edit.c" vshPrint(ctl, _("State file %s edited.\n"), file); ret = true; cleanup: - VIR_FREE(doc); - VIR_FREE(doc_edited); - if (tmp) { - unlink(tmp); - VIR_FREE(tmp); - } return ret; } @@ -8547,10 +8511,7 @@ cmdInterfaceEdit (vshControl *ctl, const vshCmd *cmd) { bool ret = false; virInterfacePtr iface = NULL; - char *tmp = NULL; - char *doc = NULL; - char *doc_edited = NULL; - char *doc_reread = NULL; + virInterfacePtr iface_edited = NULL; unsigned int flags = VIR_INTERFACE_XML_INACTIVE; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -8560,49 +8521,13 @@ cmdInterfaceEdit (vshControl *ctl, const vshCmd *cmd) if (iface == NULL) goto cleanup; - /* Get the XML configuration of the interface. */ - doc = virInterfaceGetXMLDesc (iface, flags); - if (!doc) - goto cleanup; - - /* Create and open the temporary file. */ - tmp = editWriteToTempFile (ctl, doc); - if (!tmp) goto cleanup; - - /* Start the editor. */ - if (editFile (ctl, tmp) == -1) goto cleanup; - - /* Read back the edited file. */ - doc_edited = editReadBackFile (ctl, tmp); - if (!doc_edited) goto cleanup; - - /* Compare original XML with edited. Has it changed at all? */ - if (STREQ (doc, doc_edited)) { - vshPrint (ctl, _("Interface %s XML configuration not changed.\n"), - virInterfaceGetName (iface)); - ret = true; - goto cleanup; - } - - /* Now re-read the interface XML. Did someone else change it while - * it was being edited? This also catches problems such as us - * losing a connection or the interface going away. - */ - doc_reread = virInterfaceGetXMLDesc (iface, flags); - if (!doc_reread) - goto cleanup; - - if (STRNEQ (doc, doc_reread)) { - vshError(ctl, "%s", - _("ERROR: the XML configuration was changed by another user")); - goto cleanup; - } - - /* Everything checks out, so redefine the interface. */ - virInterfaceFree (iface); - iface = virInterfaceDefineXML(ctl->conn, doc_edited, 0); - if (!iface) - goto cleanup; + #define EDIT_GET_XML virInterfaceGetXMLDesc (iface, flags) + #define EDIT_NOT_CHANGED \ + vshPrint (ctl, _("Interface %s XML configuration not changed.\n"), \ + virInterfaceGetName (iface)) + #define EDIT_DEFINE \ + iface_edited = virInterfaceDefineXML(ctl->conn, doc_edited, 0) + #include "virsh-edit.c" vshPrint (ctl, _("Interface %s XML configuration edited.\n"), virInterfaceGetName(iface)); @@ -8611,16 +8536,9 @@ cmdInterfaceEdit (vshControl *ctl, const vshCmd *cmd) cleanup: if (iface) - virInterfaceFree (iface); - - VIR_FREE(doc); - VIR_FREE(doc_edited); - VIR_FREE(doc_reread); - - if (tmp) { - unlink (tmp); - VIR_FREE(tmp); - } + virInterfaceFree(iface); + if (iface_edited) + virInterfaceFree(iface_edited); return ret; } @@ -9984,10 +9902,7 @@ cmdNWFilterEdit (vshControl *ctl, const vshCmd *cmd) { bool ret = false; virNWFilterPtr nwfilter = NULL; - char *tmp = NULL; - char *doc = NULL; - char *doc_edited = NULL; - char *doc_reread = NULL; + virNWFilterPtr nwfilter_edited = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -9996,49 +9911,14 @@ cmdNWFilterEdit (vshControl *ctl, const vshCmd *cmd) if (nwfilter == NULL) goto cleanup; - /* Get the XML configuration of the interface. */ - doc = virNWFilterGetXMLDesc (nwfilter, 0); - if (!doc) - goto cleanup; - - /* Create and open the temporary file. */ - tmp = editWriteToTempFile (ctl, doc); - if (!tmp) goto cleanup; - - /* Start the editor. */ - if (editFile (ctl, tmp) == -1) goto cleanup; - - /* Read back the edited file. */ - doc_edited = editReadBackFile (ctl, tmp); - if (!doc_edited) goto cleanup; - - /* Compare original XML with edited. Has it changed at all? */ - if (STREQ (doc, doc_edited)) { - vshPrint (ctl, _("Network filter %s XML configuration not changed.\n"), - virNWFilterGetName (nwfilter)); - ret = true; - goto cleanup; - } - - /* Now re-read the network filter XML. Did someone else change it while - * it was being edited? This also catches problems such as us - * losing a connection or the interface going away. - */ - doc_reread = virNWFilterGetXMLDesc (nwfilter, 0); - if (!doc_reread) - goto cleanup; - - if (STRNEQ (doc, doc_reread)) { - vshError(ctl, "%s", - _("ERROR: the XML configuration was changed by another user")); - goto cleanup; - } - - /* Everything checks out, so redefine the interface. */ - virNWFilterFree (nwfilter); - nwfilter = virNWFilterDefineXML(ctl->conn, doc_edited); - if (!nwfilter) - goto cleanup; + #define EDIT_GET_XML virNWFilterGetXMLDesc (nwfilter, 0) + #define EDIT_NOT_CHANGED \ + vshPrint (ctl, _("Network filter %s XML " \ + "configuration not changed.\n"), \ + virNWFilterGetName (nwfilter)) + #define EDIT_DEFINE \ + nwfilter_edited = virNWFilterDefineXML(ctl->conn, doc_edited) + #include "virsh-edit.c" vshPrint (ctl, _("Network filter %s XML configuration edited.\n"), virNWFilterGetName(nwfilter)); @@ -10047,16 +9927,9 @@ cmdNWFilterEdit (vshControl *ctl, const vshCmd *cmd) cleanup: if (nwfilter) - virNWFilterFree (nwfilter); - - VIR_FREE(doc); - VIR_FREE(doc_edited); - VIR_FREE(doc_reread); - - if (tmp) { - unlink (tmp); - VIR_FREE(tmp); - } + virNWFilterFree(nwfilter); + if (nwfilter_edited) + virNWFilterFree(nwfilter_edited); return ret; } @@ -15806,10 +15679,7 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) { bool ret = false; virDomainPtr dom = NULL; - char *tmp = NULL; - char *doc = NULL; - char *doc_edited = NULL; - char *doc_reread = NULL; + virDomainPtr dom_edited = NULL; unsigned int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -15819,49 +15689,12 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - /* Get the XML configuration of the domain. */ - doc = virDomainGetXMLDesc (dom, flags); - if (!doc) - goto cleanup; - - /* Create and open the temporary file. */ - tmp = editWriteToTempFile (ctl, doc); - if (!tmp) goto cleanup; - - /* Start the editor. */ - if (editFile (ctl, tmp) == -1) goto cleanup; - - /* Read back the edited file. */ - doc_edited = editReadBackFile (ctl, tmp); - if (!doc_edited) goto cleanup; - - /* Compare original XML with edited. Has it changed at all? */ - if (STREQ (doc, doc_edited)) { - vshPrint (ctl, _("Domain %s XML configuration not changed.\n"), - virDomainGetName (dom)); - ret = true; - goto cleanup; - } - - /* Now re-read the domain XML. Did someone else change it while - * it was being edited? This also catches problems such as us - * losing a connection or the domain going away. - */ - doc_reread = virDomainGetXMLDesc (dom, flags); - if (!doc_reread) - goto cleanup; - - if (STRNEQ (doc, doc_reread)) { - vshError(ctl, - "%s", _("ERROR: the XML configuration was changed by another user")); - goto cleanup; - } - - /* Everything checks out, so redefine the domain. */ - virDomainFree (dom); - dom = virDomainDefineXML(ctl->conn, doc_edited); - if (!dom) - goto cleanup; + #define EDIT_GET_XML virDomainGetXMLDesc (dom, flags) + #define EDIT_NOT_CHANGED \ + vshPrint (ctl, _("Domain %s XML configuration not changed.\n"), \ + virDomainGetName (dom)) + #define EDIT_DEFINE dom_edited = virDomainDefineXML(ctl->conn, doc_edited) + #include "virsh-edit.c" vshPrint (ctl, _("Domain %s XML configuration edited.\n"), virDomainGetName(dom)); @@ -15870,16 +15703,9 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) cleanup: if (dom) - virDomainFree (dom); - - VIR_FREE(doc); - VIR_FREE(doc_edited); - VIR_FREE(doc_reread); - - if (tmp) { - unlink (tmp); - VIR_FREE(tmp); - } + virDomainFree(dom); + if (dom_edited) + virDomainFree(dom_edited); return ret; } @@ -15899,8 +15725,41 @@ static const vshCmdOptDef opts_network_edit[] = { {NULL, 0, 0, NULL} }; -/* This is generated from this file by a sed script in the Makefile. */ -#include "virsh-net-edit.c" +static bool +cmdNetworkEdit (vshControl *ctl, const vshCmd *cmd) +{ + bool ret = false; + virNetworkPtr network = NULL; + virNetworkPtr network_edited = NULL; + unsigned int flags = 0; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + network = vshCommandOptNetwork (ctl, cmd, NULL); + if (network == NULL) + goto cleanup; + + #define EDIT_GET_XML virNetworkGetXMLDesc (network, flags) + #define EDIT_NOT_CHANGED \ + vshPrint (ctl, _("Network %s XML configuration not changed.\n"), \ + virNetworkGetName (network)) + #define EDIT_DEFINE network_edited = virNetworkDefineXML(ctl->conn, doc_edited) + #include "virsh-edit.c" + + vshPrint (ctl, _("Network %s XML configuration edited.\n"), + virNetworkGetName(network)); + + ret = true; + + cleanup: + if (network) + virNetworkFree(network); + if (network_edited) + virNetworkFree(network_edited); + + return ret; +} /* * "pool-edit" command @@ -15916,8 +15775,42 @@ static const vshCmdOptDef opts_pool_edit[] = { {NULL, 0, 0, NULL} }; -/* This is generated from this file by a sed script in the Makefile. */ -#include "virsh-pool-edit.c" +static bool +cmdPoolEdit (vshControl *ctl, const vshCmd *cmd) +{ + bool ret = false; + virStoragePoolPtr pool = NULL; + virStoragePoolPtr pool_edited = NULL; + unsigned int flags = 0; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + pool = vshCommandOptPool (ctl, cmd, "pool", NULL); + if (pool == NULL) + goto cleanup; + + #define EDIT_GET_XML virStoragePoolGetXMLDesc (pool, flags) + #define EDIT_NOT_CHANGED \ + vshPrint (ctl, _("Pool %s XML configuration not changed.\n"), \ + virStoragePoolGetName (pool)) + #define EDIT_DEFINE \ + pool_edited = virStoragePoolDefineXML(ctl->conn, doc_edited, 0) + #include "virsh-edit.c" + + vshPrint (ctl, _("Pool %s XML configuration edited.\n"), + virStoragePoolGetName(pool)); + + ret = true; + + cleanup: + if (pool) + virStoragePoolFree(pool); + if (pool_edited) + virStoragePoolFree(pool_edited); + + return ret; +} /* * "quit" command @@ -16310,9 +16203,6 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) const char *name; const char *edited_name; bool ret = false; - char *tmp = NULL; - char *doc = NULL; - char *doc_edited = NULL; unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE; unsigned int define_flags = VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; bool rename_okay = vshCommandOptBool(cmd, "rename"); @@ -16339,45 +16229,17 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) &snapshot, &name) < 0) goto cleanup; - /* Get the XML configuration of the snapshot. */ - doc = virDomainSnapshotGetXMLDesc(snapshot, getxml_flags); - if (!doc) - goto cleanup; - - /* strstr is safe here, since xml came from libvirt API and not user */ - if (strstr(doc, "<state>disk-snapshot</state>")) - define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; - - /* Create and open the temporary file. */ - tmp = editWriteToTempFile(ctl, doc); - if (!tmp) - goto cleanup; - - /* Start the editor. */ - if (editFile(ctl, tmp) == -1) - goto cleanup; - - /* Read back the edited file. */ - doc_edited = editReadBackFile(ctl, tmp); - if (!doc_edited) - goto cleanup; - - /* Compare original XML with edited. Short-circuit if it did not - * change, and we do not have any flags. */ - if (STREQ(doc, doc_edited) && - !(define_flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) { - vshPrint(ctl, _("Snapshot %s XML configuration not changed.\n"), - name); - ret = true; - goto cleanup; - } - - /* Everything checks out, so redefine the xml. */ - edited = virDomainSnapshotCreateXML(dom, doc_edited, define_flags); - if (!edited) { - vshError(ctl, _("Failed to update %s"), name); - goto cleanup; - } + #define EDIT_GET_XML \ + virDomainSnapshotGetXMLDesc(snapshot, getxml_flags); + #define EDIT_NOT_CHANGED \ + if (define_flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) \ + goto cleanup; \ + vshPrint(ctl, _("Snapshot %s XML configuration not changed.\n"), name) + #define EDIT_DEFINE \ + (strstr(doc, "<state>disk-snapshot</state>") ? \ + define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY : 0), \ + edited = virDomainSnapshotCreateXML(dom, doc_edited, define_flags) + #include "virsh-edit.c" edited_name = virDomainSnapshotGetName(edited); if (STREQ(name, edited_name)) { @@ -16406,16 +16268,12 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - VIR_FREE(doc); - VIR_FREE(doc_edited); - if (tmp) { - unlink(tmp); - VIR_FREE(tmp); - } - if (snapshot) - virDomainSnapshotFree(snapshot); if (edited) virDomainSnapshotFree(edited); + else + vshError(ctl, _("Failed to update %s"), name); + if (snapshot) + virDomainSnapshotFree(snapshot); if (dom) virDomainFree(dom); return ret; -- 1.7.8.5
On 05/18/2012 06:48 AM, Michal Privoznik wrote:
Currently, we either generate some cmd*Edit commands (cmdPoolEdit and cmdNetworkEdit) via sed script or copy the body of cmdEdit (e.g. cmdInterfaceEdit, cmdNWFilterEdit, etc.). This fact makes it harder to implement any new feature to our editing system. Therefore switch to new implementation - define macros to: - dump XML (EDIT_GET_XML) - take an action if XML wasn't changed, usually just vshPrint() (EDIT_NOT_CHANGED) - define new object (EDIT_DEFINE) - the edited XML is in @doc_edited and #include "virsh-edit.c" --- cfg.mk | 4 +- po/POTFILES.in | 1 + tools/Makefile.am | 36 +----- tools/virsh-edit.c | 69 +++++++++ tools/virsh.c | 394 +++++++++++++++++----------------------------------- 5 files changed, 199 insertions(+), 305 deletions(-) create mode 100644 tools/virsh-edit.c
MUCH nicer :)
+++ b/tools/Makefile.am @@ -111,41 +111,7 @@ virsh_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(LIBXML_CFLAGS) \ $(READLINE_CFLAGS) -BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c -
+BUILT_SOURCES = virsh-edit.c
Drop this line. virsh-edit.c should be part of libvirt.git, but BUILT_SOURCES is only for generated files that are not part of the repo.
+++ b/tools/virsh-edit.c @@ -0,0 +1,69 @@
Needs a copyright header. Also needs comments describing how to use the file (that is, that this is a file fragment designed for inclusion from other .c files, and document the macro names that must exist prior to inclusion). It might also be worth doing: #ifndef EDIT_GET_XML # error Missing EDIT_GET_XML definition #endif and so forth, for sanity checking.
+do { + char *tmp = NULL; + char *doc = NULL; + char *doc_edited = NULL; + char *doc_reread = NULL; + + /* Get the XML configuration of the domain. */ + doc = EDIT_GET_XML;
This must be an expression...
+ if (!doc) + goto edit_cleanup; + + /* Create and open the temporary file. */ + tmp = editWriteToTempFile (ctl, doc);
As long as we are moving things, let's touch up the syntax (no space before '(').
+ if (!tmp) + goto edit_cleanup; + + /* Start the editor. */ + if (editFile (ctl, tmp) == -1)
Again.
+ goto edit_cleanup; + + /* Read back the edited file. */ + doc_edited = editReadBackFile (ctl, tmp);
Again.
+ if (!doc_edited) + goto edit_cleanup; + + /* Compare original XML with edited. Has it changed at all? */ + if (STREQ (doc, doc_edited)) {
AGAIN.
+ EDIT_NOT_CHANGED;
...whereas this can be a (possibly-compound) statement, including a break statement. Useful hints to document at the top of the file when stating what macros must exist.
+ ret = true; + goto edit_cleanup; + } + + /* Now re-read the domain XML. Did someone else change it while + * it was being edited? This also catches problems such as us + * losing a connection or the domain going away. + */ + doc_reread = EDIT_GET_XML; + if (!doc_reread) + goto edit_cleanup; + + if (STRNEQ (doc, doc_reread)) { + vshError(ctl, "%s", _("ERROR: the XML configuration " + "was changed by another user")); + goto edit_cleanup; + } + + /* Everything checks out, so redefine the domain. */ + if (!(EDIT_DEFINE)) + goto edit_cleanup; + + goto edit_continue;
A simple 'break' would get you out of the do/while(0) loop, without the need for a new label. And that's a good change, because...
+ +edit_cleanup: + VIR_FREE(doc); + VIR_FREE(doc_edited); + VIR_FREE(doc_reread); + if (tmp) { + unlink (tmp); + VIR_FREE(tmp); + } + goto cleanup; + +} while (0); + +edit_continue:
...this trailing label is risky. If the caller had done: #define ... { #include "virsh-edit.c" } leaving a trailing label would be a syntax error.
+ #define EDIT_GET_XML virNWFilterGetXMLDesc (nwfilter, 0)
Again, no space before '('.
@@ -15899,8 +15725,41 @@ static const vshCmdOptDef opts_network_edit[] = { {NULL, 0, 0, NULL} };
-/* This is generated from this file by a sed script in the Makefile. */ -#include "virsh-net-edit.c" +static bool +cmdNetworkEdit (vshControl *ctl, const vshCmd *cmd)
and again. etc. Overall, looks like you're almost there. The missing copyright is worth a v2 (we should always be in a habit of ensuring good copyright, after seeing what qemu is going through). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/18/2012 06:48 AM, Michal Privoznik wrote:
Currently, we either generate some cmd*Edit commands (cmdPoolEdit and cmdNetworkEdit) via sed script or copy the body of cmdEdit (e.g. cmdInterfaceEdit, cmdNWFilterEdit, etc.). This fact makes it harder to implement any new feature to our editing system. Therefore switch to new implementation - define macros to: - dump XML (EDIT_GET_XML) - take an action if XML wasn't changed, usually just vshPrint() (EDIT_NOT_CHANGED) - define new object (EDIT_DEFINE) - the edited XML is in @doc_edited and #include "virsh-edit.c"
+ + /* Now re-read the domain XML. Did someone else change it while + * it was being edited? This also catches problems such as us + * losing a connection or the domain going away. + */ + doc_reread = EDIT_GET_XML; + if (!doc_reread) + goto edit_cleanup;
Oh, I meant to add - in patch 2/2, this code should be made smarter. If the session is interactive, then we should give the user a three-way choice: force their edits (overwriting the changes in the meantime), reedit by starting from the new definition, or quit. Right now, we are just quitting without a choice. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 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) { + 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..1feea9e 100644 --- a/tools/console.h +++ b/tools/console.h @@ -30,6 +30,8 @@ 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-edit.c b/tools/virsh-edit.c index 060dc14..d618bfa 100644 --- a/tools/virsh-edit.c +++ 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; goto edit_cleanup; + } goto edit_continue; diff --git a/tools/virsh.c b/tools/virsh.c index fb8ede4..4cb2013 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> @@ -654,6 +655,50 @@ vshReconnect(vshControl *ctl) ctl->useSnapshotOld = false; } +/** + * 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; + + virshReportError(ctl); + + if (vshMakeStdinRaw(&ttyattr, false) < 0) + return -1; + + 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; + + ret = 1; +cleanup: + vshPrint(ctl, "\r\n"); + return ret; +} + /* --------------- * Commands * --------------- -- 1.7.8.5
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.
+ return -1;
[2] you are returning without restoring the tty to a sane state. Oops.
+ + 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..."));
+ 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). [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 * ---------------
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 * ---------------
On 05/22/2012 03:49 AM, Michal Privoznik wrote:
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
[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.
Makes sense - if we don't know how to ask the question, then it is the same as if we asked the question and the answer was successfully 'no'.
/* 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?
Eww. That does raise an interesting question. Maybe it's better to make it a two part question: 1. Simultaneous external edit detected. Continue your edit [y/n]? and if yes, 2. Discard local edits by reloading external state [y/n]? or something along those lines, where we can at a minimum reuse our yes/no parsing (and thus have only one place that needs to learn I18N in the future). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 22.05.2012 14:41, Eric Blake wrote:
On 05/22/2012 03:49 AM, Michal Privoznik wrote:
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
[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.
Makes sense - if we don't know how to ask the question, then it is the same as if we asked the question and the answer was successfully 'no'.
/* 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?
Eww. That does raise an interesting question. Maybe it's better to make it a two part question:
1. Simultaneous external edit detected. Continue your edit [y/n]?
and if yes,
2. Discard local edits by reloading external state [y/n]?
or something along those lines, where we can at a minimum reuse our yes/no parsing (and thus have only one place that needs to learn I18N in the future).
I don't like being asked twice. I think users would prefer one question with many answers, e.g. 'git add -p' produces: Stage this hunk [y,n,q,a,d,/,e,?]? So maybe: Failed. Try again [y,n,f,?]? with '?' printing out: y - yes n - no f - force to continue with my change and drop changes made meanwhile ? - print this help Michal
On 05/22/2012 09:45 AM, Michal Privoznik wrote:
Failed. [R]eedit/[S]tart over again/[Q]uit?
Eww. That does raise an interesting question. Maybe it's better to make it a two part question:
I don't like being asked twice. I think users would prefer one question with many answers, e.g. 'git add -p' produces:
Stage this hunk [y,n,q,a,d,/,e,?]?
Hmm, good counterexample.
So maybe:
Failed. Try again [y,n,f,?]?
This is at least nicer than the [r/s/q] proposal above, in that it reuses y/n from normal parsing; maybe our yes/no parser could be parameterized to state whether extra sequences are recognized, while still allowing localization of the more common y/n responses in the future. I also like keeping 'y' and 'n' as sane defaults, in case someone ever does 'yes | virsh ...' with an expectation of always answering 'yes' being able to run the program to eventual completion.
with '?' printing out: y - yes n - no f - force to continue with my change and drop changes made meanwhile ? - print this help
I think you've persuaded me to go with this route, and not double questioning. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik