12:49 p.m.
On 05/24/2012 10:20 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 | 40 +-----
tools/virsh-edit.c | 111 ++++++++++++++
tools/virsh.c | 421 +++++++++++++++++-----------------------------------
5 files changed, 255 insertions(+), 322 deletions(-)
create mode 100644 tools/virsh-edit.c
Needs a v3. See my interleaved comments (hopefully it's obvious how I'm
pointing out two different causes and later the locations that need
fixes for those bugs).
+ *
+ * EDIT_DEFINE - which redefines the object. The edited XML from user is in
+ * 'doc_edited' variable. Don't overwrite the pointer to the object,
+ * as we may iterate once more over and therefore the pointer would be
+ * invalid. Hence assign object to a different variable.
+ * Moreover, this needs to be an expression where:
+ * - 0 is taken as error (our virDefine* APIs often return NULL on error)
+ * - everything else is taken as success
+ * For example:
+ * #define EDIT_DEFINE dom_edited = virDomainDefineXML(ctl->conn, doc_edited)
This recommendation is a memory leak - if you call EDIT_DEFINE more than
once, then the second call will overwrite the dom_edited from the first
call. The fix is to add a fourth macro, EDIT_FREE, which is called as a
statement [1]...
+ *
+ * Michal Privoznik <mprivozn(a)redhat.com>
+ */
+
+#ifndef EDIT_GET_XML
+# error Missing EDIT_GET_XML definition
+#endif
+
+#ifndef EDIT_NOT_CHANGED
+# error Missing EDIT_NOT_CHANGED definition
+#endif
+
+#ifndef EDIT_DEFINE
+# error Missing EDIT_DEFINE definition
+#endif
+
+do {
+ /* 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;
+ }
You have a bug here. This _always_ skips the redefine phase, but at
least one caller[2]...
+
+ /* Now re-read the object XML. Did someone else change it while
+ * it was being edited? This also catches problems such as us
+ * losing a connection or the object 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. */
[1]...right here.
+ if (!(EDIT_DEFINE))
+ goto edit_cleanup;
+
+ break;
+
+edit_cleanup:
+ VIR_FREE(doc);
+ VIR_FREE(doc_edited);
+ VIR_FREE(doc_reread);
+ if (tmp) {
+ unlink (tmp);
+ VIR_FREE(tmp);
+ }
+ goto cleanup;
+
+} while (0);
+
+
+#undef EDIT_GET_XML
+#undef EDIT_NOT_CHANGED
+#undef EDIT_DEFINE
diff --git a/tools/virsh.c b/tools/virsh.c
- if (!tmp) goto cleanup;
-
- /* Start the editor. */
- if (editFile (ctl, tmp) == -1) 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"
[1] Here, EDIT_FREE would be /* */ (no extra work, as no extra resources
are allocated).
+ #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)
Here, EDIT_FREE would be:
if (iface_edited)
virInterfaceFree (iface_edited);
and so forth.
-
- /* 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;
- }
[2]...has semantics where the presence of a flag MUST trigger a
redefine, even if the XML didn't change, but your replacement...
-
- /* 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)
[2]...is not following those semantics. Perhaps the right fix is to
change 'EDIT_NOT_CHANGED' to have the caller be in control of issuing
'ret=true; goto cleanup;' as appropriate, rather than blindly doing that
for all callers in the include file. Then this particular definition
would be:
#define EDIT_NOT_CHANGED \
/* Depending on flags, we re-edit even if XML is unchanged. */ \
if (!define_flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) { \
vshPrint(ctl, \
_("Snapshot %s XML configuration not changed.\n"), \
name); \
ret = true; \
goto cleanup; \
}
while most other definitions skip the if(){} and just have the three
statements.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org