"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