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

[Jim Meyering]

"Richard W.M. Jones" <rjones at 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