[libvirt] [PATCH v3 1/2] virsh: Switch from generated cmd*Edit commands to nongenerated

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120601/01f9eef6/attachment-0001.sig>