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@xxxxxxxxxx>
> + */
> +
> +#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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list