12:03 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org