"Richard W.M. Jones" <rjones@xxxxxxxxxx> 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list