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@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