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

[Eric Blake]

On 05/18/2012 06:48 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  |   36 +-----
>  tools/virsh-edit.c |   69 +++++++++
>  tools/virsh.c      |  394 +++++++++++++++++-----------------------------------
>  5 files changed, 199 insertions(+), 305 deletions(-)
>  create mode 100644 tools/virsh-edit.c

MUCH nicer :)

> +++ b/tools/Makefile.am
> @@ -111,41 +111,7 @@ virsh_CFLAGS =							\
>  		$(COVERAGE_CFLAGS)				\
>  		$(LIBXML_CFLAGS)				\
>  		$(READLINE_CFLAGS)
> -BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c
> -

> +BUILT_SOURCES = virsh-edit.c

Drop this line.  virsh-edit.c should be part of libvirt.git, but
BUILT_SOURCES is only for generated files that are not part of the repo.

> +++ b/tools/virsh-edit.c
> @@ -0,0 +1,69 @@

Needs a copyright header.  Also needs comments describing how to use the
file (that is, that this is a file fragment designed for inclusion from
other .c files, and document the macro names that must exist prior to
inclusion).  It might also be worth doing:

#ifndef EDIT_GET_XML
# error Missing EDIT_GET_XML definition
#endif

and so forth, for sanity checking.

> +do {
> +    char *tmp = NULL;
> +    char *doc = NULL;
> +    char *doc_edited = NULL;
> +    char *doc_reread = NULL;
> +
> +    /* Get the XML configuration of the domain. */
> +    doc = EDIT_GET_XML;

This must be an expression...

> +    if (!doc)
> +        goto edit_cleanup;
> +
> +    /* Create and open the temporary file. */
> +    tmp = editWriteToTempFile (ctl, doc);

As long as we are moving things, let's touch up the syntax (no space
before '(').

> +    if (!tmp)
> +        goto edit_cleanup;
> +
> +    /* Start the editor. */
> +    if (editFile (ctl, tmp) == -1)

Again.

> +        goto edit_cleanup;
> +
> +    /* Read back the edited file. */
> +    doc_edited = editReadBackFile (ctl, tmp);

Again.

> +    if (!doc_edited)
> +        goto edit_cleanup;
> +
> +    /* Compare original XML with edited.  Has it changed at all? */
> +    if (STREQ (doc, doc_edited)) {

AGAIN.

> +        EDIT_NOT_CHANGED;

...whereas this can be a (possibly-compound) statement, including a
break statement.  Useful hints to document at the top of the file when
stating what macros must exist.

> +        ret = true;
> +        goto edit_cleanup;
> +    }
> +
> +    /* Now re-read the domain XML.  Did someone else change it while
> +     * it was being edited?  This also catches problems such as us
> +     * losing a connection or the domain 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. */
> +    if (!(EDIT_DEFINE))
> +        goto edit_cleanup;
> +
> +    goto edit_continue;

A simple 'break' would get you out of the do/while(0) loop, without the
need for a new label.  And that's a good change, because...

> +
> +edit_cleanup:
> +    VIR_FREE(doc);
> +    VIR_FREE(doc_edited);
> +    VIR_FREE(doc_reread);
> +    if (tmp) {
> +        unlink (tmp);
> +        VIR_FREE(tmp);
> +    }
> +    goto cleanup;
> +
> +} while (0);
> +
> +edit_continue:

...this trailing label is risky.  If the caller had done:

#define ...
{
#include "virsh-edit.c"
}

leaving a trailing label would be a syntax error.


> +    #define EDIT_GET_XML virNWFilterGetXMLDesc (nwfilter, 0)

Again, no space before '('.

> @@ -15899,8 +15725,41 @@ static const vshCmdOptDef opts_network_edit[] = {
>      {NULL, 0, 0, NULL}
>  };
>  
> -/* This is generated from this file by a sed script in the Makefile. */
> -#include "virsh-net-edit.c"
> +static bool
> +cmdNetworkEdit (vshControl *ctl, const vshCmd *cmd)

and again.  etc.

Overall, looks like you're almost there.  The missing copyright is worth
a v2 (we should always be in a habit of ensuring good copyright, after
seeing what qemu is going through).

-- 
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/20120518/b501ad19/attachment-0001.sig>