Re: [libvirt] [PATCH] virsh edit, virsh net-edit, virsh pool-edit

1 Aug 2008

"Richard W.M. Jones" <rjones@redhat.com> wrote:
...
This implements 'virsh edit', 'virsh net-edit' and 'virsh pool-edit'
commands.
Hi Rich,
...
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libvirt/src/virsh.c,v
retrieving revision 1.157
diff -u -r1.157 virsh.c

--- src/virsh.c	22 Jul 2008 16:12:01 -0000	1.157
+++ src/virsh.c	30 Jul 2008 09:54:46 -0000
@@ -5070,6 +5070,424 @@
 }
/*
+ * "edit" command
+ */
These declarations can go farther down, right above the definition
of cmdEdit.
...
+static vshCmdInfo info_edit[] = {
+    {"syntax", "edit <domain>"},
+    {"help", gettext_noop("edit XML configuration for a domain")},
+    {"desc", gettext_noop("Edit the XML configuration for a domain.")},
+    {NULL, NULL}
+};
+
+static vshCmdOptDef opts_edit[] = {
You can mark entries as "const":

  static const vshCmdOptDef opts_edit[] = {
...
+    {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")},
+    {NULL, 0, 0, NULL}
+};
+
+static char *
+editWriteToTempFile (vshControl *ctl, const char *doc)
+{
+    char *ret;
+    int fd;
+
+    ret = tempnam (NULL, "virsh");
Don't use tempnam.
It poses a security risk (symlink attack in a race to open).
Use mkstemp instead, but you'll have to form the concatenation
of $TMPDIR (if defined, else /tmp) with a template like /virshXXXXXX.
...
+    if (!ret) {
+        vshError(ctl, FALSE,
+                 _("tempnam: failed to create temporary file: %s"),
+                 strerror (errno));
+        return NULL;
+    }
+
+    fd = open (ret, O_EXCL|O_CREAT|O_WRONLY, 0600);
+    if (fd == -1) {
+        vshError(ctl, FALSE,
+                 _("open: %s: failed to create temporary file: %s"),
+                 ret, strerror (errno));
+        free (ret);
+        return NULL;
+    }
+
+    if (safewrite (fd, doc, strlen (doc)) == -1) {
+        vshError(ctl, FALSE,
+                 _("write: %s: failed to create temporary file: %s"),
s/create/write/
...
+                 ret, strerror (errno));
+        close (fd);
+        unlink (ret);
+        free (ret);
+        return NULL;
+    }
+    if (close (fd) == -1) {
+        vshError(ctl, FALSE,
+                 _("close: %s: failed to create temporary file: %s"),
s/create/write/
...
+                 ret, strerror (errno));
+        unlink (ret);
+        free (ret);
+        return NULL;
+    }
+
+    /* Temporary filename: caller frees. */
+    return ret;
+}
+
+static int
+editFile (vshControl *ctl, const char *filename)
+{
+    const char *editor;
+    char command[100];
+    int command_ret;
+
+    editor = getenv ("EDITOR");
+    if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */
+
+    snprintf (command, sizeof command, "%s %s", editor, filename);
Use asprintf rather than a fixed-sized buffer.
Otherwise, if EDITOR and/or TMPDIR are long enough, the result won't fit.

What if either contains shell meta-characters?
To accommodate you'd have to shell-quote as needed, or (as I prefer)
simply detect the bogosity and refuse to run the command.
...
+    command_ret = system (command);
+
+    if (command_ret == -1) {
+        vshError(ctl, FALSE,
+                 "%s: %s",
+                 command, strerror (errno));
+        return -1;
+    }
+    if (command_ret != WEXITSTATUS (0)) {
+        vshError(ctl, FALSE,
+                 _("%s: command exited with non-zero status"), command);
+        return -1;
+    }
+    return 0;
+}
+
+static char *
+editReadBackFile (vshControl *ctl, const char *filename)
+{
...
How about using virFileReadAll here?
Or at least fread_file_lim, if virFileReadAll's error-handling
isn't exactly what you want.
...
+}
...
+static int
+cmdEdit (vshControl *ctl, vshCmd *cmd)
+{
...
+static int
+cmdNetworkEdit (vshControl *ctl, vshCmd *cmd)
+{
...
+static int
+cmdPoolEdit (vshControl *ctl, vshCmd *cmd)
+{
Have you considered factoring these three?
At 70-80 lines each, with so much common code, it's begging for it.

Two approaches:

Pure C would require lots of ugly casts and would end up
worse than the duplication.

However, consider putting this into a new file, say virsh-dom-edit.c
(or better, just extracting it from virsh.c)

    static vshCmdInfo info_edit[] = { ...
    static vshCmdOptDef opts_edit[] = { ...
    static int
    cmdEdit (vshControl *ctl, vshCmd *cmd)
    {
    ...
    }

With that, we could use a few sed substitutions to automatically derive
virsh-net-edit.c and virsh-pool-edit.c, and then replace the two +80-line
blocks of duplicate code with these two lines:

    #include "virsh-net-edit.c"
    #include "virsh-pool-edit.c"

If you do that, you'll have to add both new file names to the
list of built sources in src/Makefile.am:

    BUILT_SOURCES += virsh-net-edit.c virsh-pool-edit.c

    

Re: [libvirt] [PATCH] virsh edit, virsh net-edit, virsh pool-edit

Jim Meyering