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