On 06/01/2012 07:01 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
> - free object defined by EDIT_DEFINE (EDIT_FREE)
> and #include "virsh-edit.c"
> ---
> + * Usage:
> + * Define macros:
> + * EDIT_GET_XML - expression which produces a pointer to XML string, e.g:
> + * #define EDIT_GET_XML virDomainGetXMLDesc(dom, flags)
> + *
> + * EDIT_NOT_CHANGED - this action is taken if the XML wasn't changed.
> + * Note, that you don't want to jump to cleanup but edit_cleanp label
s/cleanp/cleanup/
> + * where temporary variables are free()-d and temporary file is deleted:
> + * #define EDIT_NOT_CHANGED vshPrint(ctl, _("Domain %s XML not changed"), \
> + * virDomainGetName(dom)); \
> + * ret = true; goto edit_cleanup;
> + * Note that this is a statement.
> + *
> + * EDIT_DEFINE - expression 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)
I'm generally a fan of making expression macros properly parenthesized
for use elsewhere, rather than making the use of the macro have to deal
with it. That means this should be:
#define EDIT_DEFINE (dom_edited = virDomainDefineXML(ctl->conn, doc_edited))
> + /* Compare original XML with edited. Has it changed at all? */
> + if (STREQ(doc, doc_edited)) {
> + EDIT_NOT_CHANGED
This looks weird. I'd put a trailing semicolon here.
> +
> + /* Everything checks out, so redefine the object. */
> + EDIT_FREE
Again, this looks weird.
My findings this time were only syntactic nits, so need to post a v4.
ACK, and looking forward to using this!
--
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