[PATCH 0/7] Lots of cleanups, don't run the daemon as root, and connect to libvirt readonly

newer
[PATCH 0/2] vm-dump-metrics: Fix...

older
[PATCH] Further changes to avoid...

Richard W.M. Jones

15 Oct 2009 15 Oct '09

12:18

This patch series is basically a lot of cleanups and should also enhance the security of vhostmd. We should allow administrators to run the daemon as root, we should always connect to libvirt readonly, and we should ensure that the libvirt URI is specified. (Libvirt's heuristics where it tries to guess the default hypervisor are suspect so don't rely on them). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Show replies by date

Richard W.M. Jones

15 Oct 15 Oct

12:18

New subject: [PATCH 1/7] autotools: Use AC_GNU_SOURCE and config.h

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html

Jim Fehlig

15:23

New subject: [PATCH 1/7] autotools: Use AC_GNU_SOURCE and config.h

Richard W.M. Jones wrote:

...

From d3958dc76cff61f0ed7563729dc3c2f2e3e3ae10 Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 11:05:28 +0100 Subject: [PATCH 1/8] autotools: Use AC_GNU_SOURCE and config.h

Use AC_GNU_SOURCE instead of coding -D_GNU_SOURCE directly.

All C files should include as the first line (before any other includes or defines). --- configure.ac | 4 +++- libmetrics/host_metrics.c | 2 ++ libmetrics/libmetrics.c | 4 +--- libmetrics/vm_metrics.c | 2 ++ test/main.c | 2 ++ vhostmd/metric.c | 2 ++ vhostmd/util.c | 2 ++ vhostmd/vhostmd.c | 2 ++ vhostmd/virt-util.c | 2 ++ vhostmd/xen-metrics.c | 2 ++ vhostmd/xenctrl-util.c | 2 ++ vhostmd/xenstore-update.c | 2 ++ vm-dump-metrics/main.c | 2 ++ 13 files changed, 26 insertions(+), 4 deletions(-)

Good cleanup, ACK. Jim -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

12:20

New subject: [PATCH 2/7] Allow gcc to check calls to vu_log* printf-like functions.

Although using gcc-specific extensions may be suspect, we now also use them in libguestfs without any problems. Lots of non-gcc compilers understand __attribute__ because so much code is now compiled for gcc. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.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

Jim Fehlig

15:25

New subject: [PATCH 2/7] Allow gcc to check calls to vu_log* printf-like functions.

Richard W.M. Jones wrote:

...

Although using gcc-specific extensions may be suspect, we now also use them in libguestfs without any problems. Lots of non-gcc compilers understand __attribute__ because so much code is now compiled for gcc.

Rich.

From ff676595c8736b0463e628fe226ca3cc437703ee Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 12:18:34 +0100 Subject: [PATCH 2/8] Allow gcc to check calls to vu_log* printf-like functions.

--- include/util.h | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/util.h b/include/util.h index 1eebf45..bd347bf 100644 --- a/include/util.h +++ b/include/util.h @@ -62,7 +62,8 @@ void vu_log_close(void); /* * Logging function. */ -void vu_log(int priority, const char *fmt, ...); +void vu_log(int priority, const char *fmt, ...) + __attribute__((format (printf, 2, 3)));

/* * Create buffer capable of holding len content. @@ -82,7 +83,8 @@ void vu_buffer_add(vu_buffer *buf, const char *str, int len); /* * Do a formatted print to buffer. */ -void vu_buffer_vsprintf(vu_buffer *buf, const char *format, ...); +void vu_buffer_vsprintf(vu_buffer *buf, const char *format, ...) + __attribute__((format (printf, 2, 3)));

/* * Erase buffer, setting use to 0 and clearing content. -- 1.6.5.rc2

ACK Jim -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

12:20

New subject: [PATCH 3/7] Clean up VM metrics error message.

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw

Jim Fehlig

15:31

New subject: [PATCH 3/7] Clean up VM metrics error message.

Richard W.M. Jones wrote:

...

From fe2861c25bad25e3966e601f0d9dc0c6c8648cc9 Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 13:09:17 +0100 Subject: [PATCH 3/8] Clean up VM metrics error message.

Don't print "Failed to collect vm metrics during update" unless there was an actual error collecting VM metrics. There being no running domains is not an error. --- vhostmd/vhostmd.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/vhostmd/vhostmd.c b/vhostmd/vhostmd.c index 741caff..1e3378d 100644 --- a/vhostmd/vhostmd.c +++ b/vhostmd/vhostmd.c @@ -870,12 +870,16 @@ static int metrics_vms_get(vu_buffer *buf, int **ids) *ids = NULL;

num_vms = vu_num_vms(); - if (num_vms < 1) + if (num_vms == -1) + return -1; + if (num_vms == 0) return 0;

*ids = calloc(num_vms, sizeof(int)); - if (*ids == NULL) - return 0; + if (*ids == NULL) { + vu_log (VHOSTMD_ERR, "calloc: %m");

Heh, I've never seen use of '%m'. Had to look that one up in man page - good to know :-). ACK Jim

...

+ return -1; + }

num_vms = vu_get_vms(*ids, num_vms); for (i = 0; i < num_vms; i++) { @@ -911,7 +915,7 @@ static int vhostmd_run(int diskfd) vu_log(VHOSTMD_ERR, "Failed to collect host metrics " "during update");

- if ((num_vms = metrics_vms_get(buf, &ids)) == 0) + if ((num_vms = metrics_vms_get(buf, &ids)) == -1) vu_log(VHOSTMD_ERR, "Failed to collect vm metrics " "during update");

-- 1.6.5.rc2

-- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

15:38

New subject: [PATCH 3/7] Clean up VM metrics error message.

On Thu, Oct 15, 2009 at 09:31:32AM -0600, Jim Fehlig wrote:

...

Heh, I've never seen use of '%m'. Had to look that one up in man page - good to know :-).

Right - A lot of the code at the moment is doing: "...%s...", strerror (errno) Technically that is more portable, but not thread safe (if we ever decided to use threads). Doing it correctly with strerror_r is very much more complicated. "%m" is both thread safe and easy to use but not (yet) portable beyond glibc. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.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 -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

12:21

New subject: [PATCH 4/7] Always connect to libvirt read-only.

This patch doesn't fix up every instance where we connect to libvirt read-write, because the configuration file contains direct calls to 'virsh' too. That is fixed up in a later patch in this series. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.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/

Jim Fehlig

15:33

New subject: [PATCH 4/7] Always connect to libvirt read-only.

Richard W.M. Jones wrote:

...

This patch doesn't fix up every instance where we connect to libvirt read-write, because the configuration file contains direct calls to 'virsh' too. That is fixed up in a later patch in this series.

Rich.

From 0c87800038cc907cd67c92ce5101596712e3d56f Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 12:30:34 +0100 Subject: [PATCH 4/8] Always connect to libvirt read-only.

We should never be modifying domains, so there should never be a need to make a writable connection to libvirt.

This also tidies up the connection code into a single function. --- vhostmd/virt-util.c | 43 +++++++++++++++++-------------------------- 1 files changed, 17 insertions(+), 26 deletions(-)

ACK Jim -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

12:22

New subject: [PATCH 5/7] Add '-u user' command line option to drop root privs.

Jim Fehlig

15:36

New subject: [PATCH 5/7] Add '-u user' command line option to drop root privs.

Richard W.M. Jones wrote:

...

From 29d73f4e62c0cd9ed6a7616f4a82d6fdf0421c06 Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 12:10:15 +0100 Subject: [PATCH 5/8] Add '-u user' command line option to drop root privs.

If vhostmd is run as root, you can now use the '-u user' command line option to drop root privs and run as 'user' instead. --- docs/man/vhostmd.8 | 3 +++ vhostmd/vhostmd.c | 51 +++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 48 insertions(+), 6 deletions(-)

Nice addition! ACK. Jim -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

15:55

New subject: [PATCH 5/7] Add '-u user' command line option to drop root privs.

On Thu, Oct 15, 2009 at 09:36:34AM -0600, Jim Fehlig wrote:

...

Richard W.M. Jones wrote:

...
From 29d73f4e62c0cd9ed6a7616f4a82d6fdf0421c06 Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 12:10:15 +0100 Subject: [PATCH 5/8] Add '-u user' command line option to drop root privs.

If vhostmd is run as root, you can now use the '-u user' command line option to drop root privs and run as 'user' instead. --- docs/man/vhostmd.8 | 3 +++ vhostmd/vhostmd.c | 51 +++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 48 insertions(+), 6 deletions(-)

Nice addition! ACK.

Thanks. I've pushed up to this point now. However I changed: + errno = 0; + pw = getpwnam (user); + if (!pw) { + perror (user); + goto out; + } to: + errno = 0; + pw = getpwnam (user); + if (!pw) { + vu_log (VHOSTMD_ERR, "No entry in password file for user %s: %m", + user); + goto out; + } This is because the 'perror' would be sent to stderr (not to syslog), and thus the error message would most likely be lost. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

12:22

New subject: [PATCH 6/7] Add '-c uri' command line option to specify libvirt

Jim Fehlig

15:38

New subject: [PATCH 6/7] Add '-c uri' command line option to specify libvirt

Richard W.M. Jones wrote:

...

From e7b1d3493af13c5fcbcbf43ff179f863fa09bf7f Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 12:27:52 +0100 Subject: [PATCH 6/8] Add '-c uri' command line option to specify libvirt connection URI.

--- docs/man/vhostmd.8 | 6 ++++++ include/util.h | 4 ++++ vhostmd/vhostmd.c | 7 ++++++- vhostmd/virt-util.c | 7 +++++-- 4 files changed, 21 insertions(+), 3 deletions(-)

ACK Jim -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

12:23

New subject: [PATCH 7/7] Add CONNECT substitution <action>s.

Richard W.M. Jones

13:06

New subject: [PATCH 7/7 v2] Add CONNECT substitution <action>s.

Sorry, that previous version in fact had a memory leak in the replace function ... Updated version. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.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/

Richard W.M. Jones

17:01

New subject: [PATCH 7/7 v3] Add CONNECT substitution <action>s.

Jim pointed out another case where there could be a memory leak along an error path. Update patch attached. GC FTW ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.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

Jim Fehlig

17:09

New subject: [PATCH 7/7 v3] Add CONNECT substitution <action>s.

Richard W.M. Jones wrote:

...

Jim pointed out another case where there could be a memory leak along an error path. Update patch attached.

GC FTW ...

Rich.

From ea2f772da718adb868150e9c4074ad49f0c374cb Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Thu, 15 Oct 2009 13:10:18 +0100 Subject: [PATCH 1/2] Add CONNECT substitution <action>s.

Allow CONNECT to be substituted in any <action>s in order to pass the correct libvirt URI down to actions such as virsh. The correct way to run virsh is now:

virsh -r CONNECT cmd [...]

which substitutes as either:

virsh -r --connect 'uri' cmd [...]

or:

virsh -r cmd [...]

Note that you should always run virsh readonly (-r option).

Also corrects the documentation: ID should be VMID.

Also this cleans up the string replacement code and ensures that it doesn't leak memory on failure. --- docs/man/vhostmd.8 | 24 ++++++++++--- vhostmd.xml | 6 ++-- vhostmd/metric.c | 97 +++++++++++++++++++++++++++++++++++---------------- 3 files changed, 87 insertions(+), 40 deletions(-)

ACK Jim -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

Richard W.M. Jones

12:24

New subject: [PATCH 0/7] Lots of cleanups, don't run the daemon as root, and connect to libvirt readonly

On Thu, Oct 15, 2009 at 01:18:03PM +0100, Richard W.M. Jones wrote:

...

We should allow administrators to run the daemon as root,

I mean, of course, "as non-root". Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.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/ -- To unsubscribe, e-mail: vhostmd+unsubscribe@opensuse.org For additional commands, e-mail: vhostmd+help@opensuse.org

5309

Age (days ago)

5309

Last active (days ago)

List overview

Download

19 comments

2 participants

participants (2)

Jim Fehlig
Richard W.M. Jones