4:58 a.m.
This implements 'virsh edit', 'virsh net-edit' and 'virsh
pool-edit'
commands.
Previous discussion of this patch was in this thread:
https://www.redhat.com/archives/libvir-list/2008-July/thread.html#00434
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/
3:02 a.m.
"Richard W.M. Jones" <rjones(a)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
4:16 a.m.
On Fri, Aug 01, 2008 at 10:02:50AM +0200, Jim Meyering wrote:
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
> +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.
I certainly did!
Normally of course I'd do it with closures, but ...
I'll go with the 'sed' approach unless anyone stronly objects to that.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
5:36 a.m.
New subject: [PATCH] virsh edit (v3) (was: Re: [libvirt] [PATCH] virsh edit, virsh net-edit, virsh pool-edit)
Thanks for reviewing this patch.
On Fri, Aug 01, 2008 at 10:02:50AM +0200, Jim Meyering wrote:
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.
Yes, not quite sure what I was thinking of there. The new version
should reject characters in both the editor & filename which are
not in a small recognized set of characters.
You can mark entries as "const":
static const vshCmdOptDef opts_edit[] = {
This gives an error because the vshCmdDef array where we aggregate all
of these doesn't expect the const pointer. Obviously it's a general
const-correctness problem which needs a separate patch to fix
throughout the whole file.
A new patch is attached which should address everything that you
mentioned.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
7:44 a.m.
New subject: [libvirt] Re: [PATCH] virsh edit (v3)
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
A new patch is attached which should address everything that you
mentioned.
That was quick ;-)
Index: src/Makefile.am
===================================================================
RCS file: /data/cvs/libvirt/src/Makefile.am,v
retrieving revision 1.86
diff -u -r1.86 Makefile.am
--- src/Makefile.am 11 Jul 2008 16:23:36 -0000 1.86
+++ src/Makefile.am 1 Aug 2008 10:33:22 -0000
@@ -138,6 +138,31 @@
virsh_DEPENDENCIES = $(DEPS)
virsh_LDADD = $(LDADDS) $(VIRSH_LIBS)
virsh_CFLAGS = $(COVERAGE_CFLAGS) $(READLINE_CFLAGS)
+BUILT_SOURCES = virsh-net-edit.c virsh-pool-edit.c
+EXTRA_DIST += virsh-net-edit.c virsh-pool-edit.c
+
+virsh-net-edit.c: virsh.c Makefile.am
+ echo '/* Automatically generated from the Makefile and virsh.c */' > $@
+ echo 'static int' >> $@
+ awk '/^cmdEdit/, /^}/' $< | \
+ sed -e 's/domain/network/g' \
+ -e 's/Domain/Network/g' \
+ -e 's/cmdEdit/cmdNetworkEdit/g' \
+ -e 's/dom/network/g' \
+ >> $@
Don't redirect directly to $@.
Otherwise, if something goes wrong before the output is completely written,
you're left with a corrupt input that has an up-to-date time stamp.
That can be a pain to diagnose.
Also, if you make each generated file read-only, then even those
who don't spot the Auto-Generated-from comment at the top will have
a hard time accidentally changing it.
Also, you can include dependent names in the comment with "$^":
I indented the "sed" command so it's clearer that it's
on a backslash-continued line:
virsh-net-edit.c: virsh.c Makefile.am
rm -f $@-t $@
echo '/* Automatically generated from: $^ */' > $@-t
echo 'static int' >> $@-t
awk '/^cmdEdit/, /^}/' $< \
| sed -e 's/domain/network/g' \
-e 's/Domain/Network/g' \
-e 's/cmdEdit/cmdNetworkEdit/g' \
-e 's/dom/network/g' \
> $@-t
chmod a-w $@-t
mv $@-t $@
This is longer than the original rule, but it's for a good cause:
the point is to better protect readers/builders/developers.
We incur the cost of writing Makefile.am once, but the audience
will be saved some small wasted effort over and over ;-)
Yes, there are many other rules that can benefit from the same
treatment. have to start somewhere.
9:21 a.m.
New subject: [libvirt] [PATCH] virsh edit (v4)
Here's version 4 of the patch, which I believe covers all the things
you've raised. The changes since version 3 are:
(a) 'sed' snippets reformulated as you suggested
(b) const-correctness changes added to this patch
(c) We don't include the generated files virsh-net-edit.c or
virsh-pool-edit.c in CVS (or in the tarball).
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
9:25 a.m.
New subject: [libvirt] Re: [PATCH] virsh edit (v4)
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
Here's version 4 of the patch, which I believe covers all the
things
you've raised. The changes since version 3 are:
(a) 'sed' snippets reformulated as you suggested
(b) const-correctness changes added to this patch
(c) We don't include the generated files virsh-net-edit.c or
virsh-pool-edit.c in CVS (or in the tarball).
Thanks! All good.
ACK
5956
days inactive
5958
days old
6 comments
2 participants
participants (2)
-
Jim Meyering -
Richard W.M. Jones