[libvirt] adding tests....
In adding a couple of tests, I noticed that libvirtd --config=no-such didn't diagnose my mistake. I fixed that with the first patch below: diagnose "libvirtd --config=no-such-file" * qemud/qemud.c (remoteReadConfigFile): Don't return 0 (success) when the config file is unreadable or nonexistent Return -1, not 0, upon virConfReadFile failure. (main): If remote_config_file is not specified via --config(-f), use the default config file only if it exists. Otherwise, use /dev/null. However, that made it so libvirtd gave two diagnostics: Failed to open file 'no-such': No such file or directory libvir: Config error : failed to open no-such for reading The latter part of that patch fixes it like this: * src/conf.c (virConfReadFile): Don't diagnose virFileReadAll failure, since it already does that. Finally, I went to add the two tests, one to ensure libvirtd --config=no-such-file now fails, as I expected another to start libvirtd and then run a small pool-related test via a separate virsh invocation. But that made me see a bug in tests/Makefile.am: A missing backslash made it so the virsh-all test wasn't being run. Easy to fix. But then, I saw when virsh-all runs it generated too much output, so I did this: tests: quiet virsh-all * tests/virsh-all: For now, ignore diagnostics and exit status, when running all virsh commands. Finally, here are the two tests: add tests * tests/libvirtd-pool: New file. * tests/libvirtd-fail: New file. * tests/Makefile.am (test_scripts): Add omitted backslash, so that the virsh-all test is run. Add libvirtd-fail and libvirtd-pool.
From aaea6d51f5200e58086d9cabf8b6e7fac7c460f8 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 8 Jan 2009 20:17:35 +0100 Subject: [PATCH 1/3] diagnose "libvirtd --config=no-such-file"
* qemud/qemud.c (remoteReadConfigFile): Don't return 0 (success) when the config file is unreadable or nonexistent Return -1, not 0, upon virConfReadFile failure. (main): If remote_config_file is not specified via --config(-f), use the default config file only if it exists. Otherwise, use /dev/null. * src/conf.c (virConfReadFile): Don't diagnose virFileReadAll failure, since it already does that. --- qemud/qemud.c | 20 ++++++++++++-------- src/conf.c | 3 +-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index c3d3c02..9835e0a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1,7 +1,7 @@ /* * qemud.c: daemon start of day, guest process & i/o management * - * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -2116,13 +2116,8 @@ remoteReadConfigFile (struct qemud_server *server, const char *filename) auth_unix_ro = REMOTE_AUTH_NONE; #endif - /* Just check the file is readable before opening it, otherwise - * libvirt emits an error. - */ - if (access (filename, R_OK) == -1) return 0; - conf = virConfReadFile (filename); - if (!conf) return 0; + if (!conf) return -1; /* * First get all the logging settings and activate them @@ -2301,7 +2296,7 @@ int main(int argc, char **argv) { struct sigaction sig_action; int sigpipe[2]; const char *pid_file = NULL; - const char *remote_config_file = SYSCONF_DIR "/libvirt/libvirtd.conf"; + const char *remote_config_file = NULL; int ret = 1; struct option opts[] = { @@ -2372,6 +2367,15 @@ int main(int argc, char **argv) { } } + if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + } + if (godaemon) { if (qemudGoDaemon() < 0) { VIR_ERROR(_("Failed to fork as daemon: %s"), strerror(errno)); diff --git a/src/conf.c b/src/conf.c index 9f0fc34..339a150 100644 --- a/src/conf.c +++ b/src/conf.c @@ -1,7 +1,7 @@ /** * conf.c: parser for a subset of the Python encoded Xen configuration files * - * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -712,7 +712,6 @@ virConfReadFile(const char *filename) } if ((len = virFileReadAll(filename, MAX_CONFIG_FILE_SIZE, &content)) < 0) { - virConfError(NULL, VIR_ERR_OPEN_FAILED, filename); return NULL; } -- 1.6.1.141.gfe98e
From e70a4556a8422779b7997ab03997af4b9a03087c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 8 Jan 2009 19:58:19 +0100 Subject: [PATCH 2/3] tests: quiet virsh-all
* tests/virsh-all: For now, ignore diagnostics and exit status, when running all virsh commands. --- tests/virsh-all | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/virsh-all b/tests/virsh-all index f1c84a3..03ea466 100755 --- a/tests/virsh-all +++ b/tests/virsh-all @@ -1,7 +1,7 @@ #!/bin/sh # blindly run each and every command listed by "virsh help" -# Copyright (C) 2008 Free Software Foundation, Inc. +# Copyright (C) 2008, 2009 Free Software Foundation, Inc. # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -35,7 +35,8 @@ test -n "$cmds" || framework_failure for i in $cmds; do echo testing $i... 1>&2 - virsh -c $test_url $i < /dev/null + # For now, just run the command and ignore output and exit status. + virsh -c $test_url $i < /dev/null > /dev/null 2>&1 done (exit $fail); exit $fail -- 1.6.1.141.gfe98e
From 307c465a008cbb4a497a6553046a036e95843519 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 8 Jan 2009 20:18:17 +0100 Subject: [PATCH 3/3] add two tests
* tests/libvirtd-pool: New file. * tests/libvirtd-fail: New file. * tests/Makefile.am (test_scripts): Add omitted backslash, so that the virsh-all test is run. Add libvirtd-fail and libvirtd-pool. --- tests/Makefile.am | 24 ++++++++++++---------- tests/libvirtd-fail | 20 +++++++++++++++++++ tests/libvirtd-pool | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 11 deletions(-) create mode 100755 tests/libvirtd-fail create mode 100755 tests/libvirtd-pool diff --git a/tests/Makefile.am b/tests/Makefile.am index d8b5e44..a3e4383 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -56,17 +56,19 @@ endif test_scripts = domainschematest if WITH_LIBVIRTD -test_scripts += \ - test_conf.sh \ - cpuset \ - daemon-conf \ - int-overflow \ - read-bufsiz \ - read-non-seekable \ - start \ - undefine \ - vcpupin - virsh-all +test_scripts += \ + cpuset \ + daemon-conf \ + int-overflow \ + libvirtd-fail \ + libvirtd-pool \ + read-bufsiz \ + read-non-seekable \ + start \ + test_conf.sh \ + undefine \ + vcpupin \ + virsh-all \ virsh-synopsis endif diff --git a/tests/libvirtd-fail b/tests/libvirtd-fail new file mode 100755 index 0000000..724d7c6 --- /dev/null +++ b/tests/libvirtd-fail @@ -0,0 +1,20 @@ +#!/bin/sh +# Ensure that libvirt fails when given nonexistent --config=FILE + +if test "$VERBOSE" = yes; then + set -x + libvirtd --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +libvirtd --config=no-such-file > log 2>&1 && fail=1 +cat <<\EOF > exp +Failed to open file 'no-such-file': No such file or directory +EOF + +compare exp log diff --git a/tests/libvirtd-pool b/tests/libvirtd-pool new file mode 100755 index 0000000..72afa12 --- /dev/null +++ b/tests/libvirtd-pool @@ -0,0 +1,53 @@ +#!/bin/sh +# Get coverage of libvirtd's config-parsing code. + +if test "$VERBOSE" = yes; then + set -x + libvirtd --version + virsh --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +libvirtd > log 2>&1 & pid=$! +sleep 1 + +virsh --connect qemu:///session \ + pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + || fail=1 +virsh --connect qemu:///session pool-dumpxml P >> out 2>&1 || fail=1 + +# remove random uuid +sed 's/<uuid>.*/-/' out > k && mv k out || fail=1 + +kill $pid + +cat <<EOF > exp +Pool P defined + +<pool type='dir'> + <name>P</name> + - + <capacity>0</capacity> + <allocation>0</allocation> + <available>0</available> + <source> + </source> + <target> + <path>/target-path</path> + <permissions> + <mode>0700</mode> + <owner>500</owner> + <group>500</group> + </permissions> + </target> +</pool> + +EOF + +compare exp out +compare /dev/null log -- 1.6.1.141.gfe98e
On Thu, Jan 08, 2009 at 08:55:31PM +0100, Jim Meyering wrote:
In adding a couple of tests, I noticed that libvirtd --config=no-such didn't diagnose my mistake.
I fixed that with the first patch below:
diagnose "libvirtd --config=no-such-file" * qemud/qemud.c (remoteReadConfigFile): Don't return 0 (success) when the config file is unreadable or nonexistent Return -1, not 0, upon virConfReadFile failure. (main): If remote_config_file is not specified via --config(-f), use the default config file only if it exists. Otherwise, use /dev/null.
However, that made it so libvirtd gave two diagnostics:
Failed to open file 'no-such': No such file or directory libvir: Config error : failed to open no-such for reading
The latter part of that patch fixes it like this:
* src/conf.c (virConfReadFile): Don't diagnose virFileReadAll failure, since it already does that.
Ok, makes sense.
Finally, I went to add the two tests, one to ensure libvirtd --config=no-such-file now fails, as I expected another to start libvirtd and then run a small pool-related test via a separate virsh invocation.
But that made me see a bug in tests/Makefile.am: A missing backslash made it so the virsh-all test wasn't being run. Easy to fix. But then, I saw when virsh-all runs it generated too much output, so I did this:
tests: quiet virsh-all * tests/virsh-all: For now, ignore diagnostics and exit status, when running all virsh commands.
Finally, here are the two tests:
add tests * tests/libvirtd-pool: New file. * tests/libvirtd-fail: New file. * tests/Makefile.am (test_scripts): Add omitted backslash, so that the virsh-all test is run. Add libvirtd-fail and libvirtd-pool.
ACK
@@ -2116,13 +2116,8 @@ remoteReadConfigFile (struct qemud_server *server, const char *filename) auth_unix_ro = REMOTE_AUTH_NONE; #endif
- /* Just check the file is readable before opening it, otherwise - * libvirt emits an error. - */ - if (access (filename, R_OK) == -1) return 0; - conf = virConfReadFile (filename); - if (!conf) return 0; + if (!conf) return -1;
/* * First get all the logging settings and activate them @@ -2301,7 +2296,7 @@ int main(int argc, char **argv) { struct sigaction sig_action; int sigpipe[2]; const char *pid_file = NULL; - const char *remote_config_file = SYSCONF_DIR "/libvirt/libvirtd.conf"; + const char *remote_config_file = NULL; int ret = 1;
struct option opts[] = { @@ -2372,6 +2367,15 @@ int main(int argc, char **argv) { } }
+ if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + }
Indentation looks off-by-2 there.
+virsh --connect qemu:///session \ + pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + || fail=1 +virsh --connect qemu:///session pool-dumpxml P >> out 2>&1 || fail=1
Using qemu:///session here is fragile because it'll see all existing user defined vms/network/storage/etc. Use the test:///default driver instead (or test:///path/to/custom/config.xml) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
+ if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + }
Indentation looks off-by-2 there.
Good catch. I had TABs there. Fixed.
+virsh --connect qemu:///session \ + pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + || fail=1 +virsh --connect qemu:///session pool-dumpxml P >> out 2>&1 || fail=1
Using qemu:///session here is fragile because it'll see all existing user defined vms/network/storage/etc. Use the test:///default driver instead (or test:///path/to/custom/config.xml)
I wanted to exercise a "real" drivers, not always test://. How about using an unlikely pool name instead, i.e., via this incremental: diff --git a/tests/libvirtd-pool b/tests/libvirtd-pool index 72afa12..7ff6cd9 100755 --- a/tests/libvirtd-pool +++ b/tests/libvirtd-pool @@ -16,10 +16,12 @@ fail=0 libvirtd > log 2>&1 & pid=$! sleep 1 +P=long-improbable-name-$$-$RANDOM-$PPID virsh --connect qemu:///session \ - pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + pool-define-as "$P" dir src-host /src/path /src/dev S /target-path \ + > out 2>&1 \ || fail=1 -virsh --connect qemu:///session pool-dumpxml P >> out 2>&1 || fail=1 +virsh --connect qemu:///session pool-dumpxml "$P" >> out 2>&1 || fail=1 # remove random uuid sed 's/<uuid>.*/-/' out > k && mv k out || fail=1
On Mon, Jan 12, 2009 at 01:28:41PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
+ if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + }
Indentation looks off-by-2 there.
Good catch. I had TABs there. Fixed.
+virsh --connect qemu:///session \ + pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + || fail=1 +virsh --connect qemu:///session pool-dumpxml P >> out 2>&1 || fail=1
Using qemu:///session here is fragile because it'll see all existing user defined vms/network/storage/etc. Use the test:///default driver instead (or test:///path/to/custom/config.xml)
I wanted to exercise a "real" drivers, not always test://. How about using an unlikely pool name instead, i.e., via this incremental:
That's only one issue. When you start up libvirtd it is going to run autostart on all configured vms/networks/pools. We really don't want to be booting configured VMs, and then tearing them down after a few seconds. The unit tests should restrict themselves to testing things which won't have any impact on the host system. Anything else needs to be part of a functional/integration test suite, which is better as a standalone test suite you can run on any OS with libvirt - not just a development tree, so you can test the complete system is functional Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Jan 12, 2009 at 01:28:41PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
+ if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + }
Indentation looks off-by-2 there.
Good catch. I had TABs there. Fixed.
+virsh --connect qemu:///session \ + pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + || fail=1 +virsh --connect qemu:///session pool-dumpxml P >> out 2>&1 || fail=1
Using qemu:///session here is fragile because it'll see all existing user defined vms/network/storage/etc. Use the test:///default driver instead (or test:///path/to/custom/config.xml)
I wanted to exercise a "real" drivers, not always test://. How about using an unlikely pool name instead, i.e., via this incremental:
That's only one issue. When you start up libvirtd it is going to run autostart on all configured vms/networks/pools. We really don't want to be booting configured VMs, and then tearing them down after a few seconds. The unit tests should restrict themselves to testing things which won't have any impact on the host system. Anything else needs to be part of a functional/integration test suite, which is better as a standalone test suite you can run on any OS with libvirt - not just a development tree, so you can test the complete system is functional
When I run "make check", it's almost always as a non-root user, so there's no risk of this libvirtd running system-related things. However, I guess you'd like to accommodate the root-run "make check", too? It's good to recommend against running tests as root, unless absolutely required, because of the potential risk. Along those lines, how about if I just arrange to skip this test when euid == 0 ?
On Mon, Jan 12, 2009 at 02:18:05PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Jan 12, 2009 at 01:28:41PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
+ if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + }
Indentation looks off-by-2 there.
Good catch. I had TABs there. Fixed.
+virsh --connect qemu:///session \ + pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + || fail=1 +virsh --connect qemu:///session pool-dumpxml P >> out 2>&1 || fail=1
Using qemu:///session here is fragile because it'll see all existing user defined vms/network/storage/etc. Use the test:///default driver instead (or test:///path/to/custom/config.xml)
I wanted to exercise a "real" drivers, not always test://. How about using an unlikely pool name instead, i.e., via this incremental:
That's only one issue. When you start up libvirtd it is going to run autostart on all configured vms/networks/pools. We really don't want to be booting configured VMs, and then tearing them down after a few seconds. The unit tests should restrict themselves to testing things which won't have any impact on the host system. Anything else needs to be part of a functional/integration test suite, which is better as a standalone test suite you can run on any OS with libvirt - not just a development tree, so you can test the complete system is functional
When I run "make check", it's almost always as a non-root user, so there's no risk of this libvirtd running system-related things.
However, I guess you'd like to accommodate the root-run "make check", too? It's good to recommend against running tests as root, unless absolutely required, because of the potential risk.
The QEMU driver runs as non-root too. This is what the qemu:///session URI is used for. Likewise with the UML driver. The existing tests that invoke libvirtd fail quite frequently for me already due to them activating the QEMU / UML drivers. We really need a way to explicitly say what drivers should be allowed by the daemon, overriding what's compiled in. THis could in fact be useful even for production deployment, allowing site admins to guarentee that Xen driver is never used in the daemon even if it is compiled in by default. So perhaps a couple of config params like allowed_drivers = [ "qemu", "xen", "test" ] unix_sock_dir = "/var/run/libvirt/" Not sure how best to hook the first one up to libvirt.so though - the virInitialize/virStateInitize calls always activate all of them, with no easy way to disable. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
The QEMU driver runs as non-root too. This is what the qemu:///session URI is used for. Likewise with the UML driver. The existing tests that invoke libvirtd fail quite frequently for me already due to them activating the QEMU / UML drivers. We really need a way to explicitly say what drivers should be allowed by the daemon, overriding what's compiled in. THis could in fact be useful even for production deployment, allowing site admins to guarentee that Xen driver is never used in the daemon even if it is compiled in by default.
So perhaps a couple of config params like
allowed_drivers = [ "qemu", "xen", "test" ] unix_sock_dir = "/var/run/libvirt/"
Not sure how best to hook the first one up to libvirt.so though - the virInitialize/virStateInitize calls always activate all of them, with no easy way to disable.
Sounds good. I'm deferring "allowed_drivers" for now, and preparing a patch to add support for a new configuration parameter unix_sock_dir and also for log_dir The latter is required because otherwise, there's no way for non-root to use a log file other than ~/.libvirt/log, or for root to use a log file other than the default.
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
The QEMU driver runs as non-root too. This is what the qemu:///session URI is used for. Likewise with the UML driver. The existing tests that invoke libvirtd fail quite frequently for me already due to them activating the QEMU / UML drivers. We really need a way to explicitly say what drivers should be allowed by the daemon, overriding what's compiled in. THis could in fact be useful even for production deployment, allowing site admins to guarentee that Xen driver is never used in the daemon even if it is compiled in by default.
So perhaps a couple of config params like
allowed_drivers = [ "qemu", "xen", "test" ] unix_sock_dir = "/var/run/libvirt/"
Not sure how best to hook the first one up to libvirt.so though - the virInitialize/virStateInitize calls always activate all of them, with no easy way to disable.
Sounds good. I'm deferring "allowed_drivers" for now, and preparing a patch to add support for a new configuration parameter
unix_sock_dir
and also for
log_dir
I'll add only unix_sock_dir for starters: The first patch adds the new parameter, and the second uses it in one of the two new libvirtd-running tests.
From 58a5957df136f5f38e5c400446301d28d1d2f80f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 12 Jan 2009 17:17:19 +0100 Subject: [PATCH 1/2] libvirtd: new config-file option: unix_sock_dir
Before this change, the unix socket directory was hard-coded to be e.g., /var/run/libvirt for euid==0 and ~/.libvirt otherwise. With this change, you may now specify that directory in libvirtd's config file via a line like this: unix_sock_dir = "/var/run/libvirt". This is essential for running tests that do not impinge on any existing libvirtd process, and in running tests in parallel. * qemud/libvirtd.conf (unix_sock_dir): Add comment and example. * qemud/qemud.c (unix_sock_dir): New global (remoteReadConfigFile): Set the global. (qemudInitPaths): Use the global, unix_sock_dir, if non-NULL. (main): Use the new global rather than hard-coding "/run/libvirt". * qemud/libvirtd.aug (sock_acl_entry): Add "unix_sock_dir". --- qemud/libvirtd.aug | 2 +- qemud/libvirtd.conf | 3 +- qemud/qemud.c | 78 ++++++++++++++++++++++++++++++++++++--------------- 3 files changed, 58 insertions(+), 25 deletions(-) diff --git a/qemud/libvirtd.aug b/qemud/libvirtd.aug index 7cfd458..47e96b5 100644 --- a/qemud/libvirtd.aug +++ b/qemud/libvirtd.aug @@ -35,6 +35,7 @@ module Libvirtd = let sock_acl_entry = str_entry "unix_sock_group" | str_entry "unix_sock_ro_perms" | str_entry "unix_sock_rw_perms" + | str_entry "unix_sock_dir" let authentication_entry = str_entry "auth_unix_ro" | str_entry "auth_unix_rw" @@ -77,4 +78,3 @@ module Libvirtd = . Util.stdexcl let xfm = transform lns filter - diff --git a/qemud/libvirtd.conf b/qemud/libvirtd.conf index ecb28dc..a995012 100644 --- a/qemud/libvirtd.conf +++ b/qemud/libvirtd.conf @@ -97,7 +97,8 @@ # control then you may want to relax this to: #unix_sock_rw_perms = "0770" - +# Set the name of the directory in which sockets will be found/created. +#unix_sock_dir = "/var/run/libvirt" ################################################################# # diff --git a/qemud/qemud.c b/qemud/qemud.c index 336f0a9..62790fb 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -105,6 +105,7 @@ static char *tcp_port = (char *) LIBVIRTD_TCP_PORT; static gid_t unix_sock_gid = 0; /* Only root by default */ static int unix_sock_rw_mask = 0700; /* Allow user only */ static int unix_sock_ro_mask = 0777; /* Allow world */ +static char *unix_sock_dir = NULL; #if HAVE_POLKIT static int auth_unix_rw = REMOTE_AUTH_POLKIT; @@ -640,42 +641,72 @@ static int qemudInitPaths(struct qemud_server *server, char *roSockname, int maxlen) { uid_t uid = geteuid(); + struct passwd *pw = NULL; + char *sock_dir; - if (!uid) { - if (snprintf (sockname, maxlen, "%s/run/libvirt/libvirt-sock", - LOCAL_STATE_DIR) >= maxlen) - goto snprintf_error; - - unlink(sockname); + if (unix_sock_dir) + sock_dir = unix_sock_dir; + else { + sock_dir = sockname; + if (uid == 0) { + if (snprintf (sock_dir, maxlen, "%s/run/libvirt", + LOCAL_STATE_DIR) >= maxlen) + goto snprintf_error; + } else { + if (!(pw = getpwuid(uid))) { + VIR_ERROR(_("Failed to find user record for uid '%d': %s"), + uid, strerror(errno)); + return -1; + } - if (snprintf (roSockname, maxlen, "%s/run/libvirt/libvirt-sock-ro", - LOCAL_STATE_DIR) >= maxlen) - goto snprintf_error; + if (snprintf(sock_dir, maxlen, "%s/.libvirt", pw->pw_dir) >= maxlen) + goto snprintf_error; + } + } - unlink(roSockname); + char *dir_prefix = strdup (sock_dir); + if (!dir_prefix) { + VIR_ERROR(_("failed to allocate memory for socket dir %s"), sock_dir); + return -1; + } - if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", LOCAL_STATE_DIR) >= PATH_MAX) + if (!uid) { + if (snprintf (sockname, maxlen, "%s/libvirt-sock", dir_prefix) >= maxlen + || (snprintf (roSockname, maxlen, "%s/libvirt-sock-ro", dir_prefix) + >= maxlen)) goto snprintf_error; + unlink(sockname); + unlink(roSockname); } else { - struct passwd *pw; - if (!(pw = getpwuid(uid))) { VIR_ERROR(_("Failed to find user record for uid '%d': %s"), uid, strerror(errno)); return -1; } - - if (snprintf(sockname, maxlen, "@%s/.libvirt/libvirt-sock", pw->pw_dir) >= maxlen) + if (snprintf(sockname, maxlen, "@%s/libvirt-sock", dir_prefix) >= maxlen) goto snprintf_error; + } - if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", pw->pw_dir) >= PATH_MAX) + if (!uid) { + if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", + LOCAL_STATE_DIR) >= PATH_MAX) goto snprintf_error; + } else { + if (pw == NULL && !(pw = getpwuid(uid))) { + VIR_ERROR(_("Failed to find user record for uid '%d': %s"), + uid, strerror(errno)); + return -1; + } + if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", + pw->pw_dir) >= PATH_MAX) + goto snprintf_error; + } - } /* !remote */ - + free (dir_prefix); return 0; snprintf_error: + free (dir_prefix); VIR_ERROR("%s", _("Resulting path too long for buffer in qemudInitPaths()")); return -1; } @@ -2183,6 +2214,8 @@ remoteReadConfigFile (struct qemud_server *server, const char *filename) unix_sock_rw_perms = NULL; } + GET_CONF_STR (conf, filename, unix_sock_dir); + GET_CONF_INT (conf, filename, mdns_adv); GET_CONF_STR (conf, filename, mdns_name); @@ -2427,11 +2460,10 @@ int main(int argc, char **argv) { goto error2; /* Change the group ownership of /var/run/libvirt to unix_sock_gid */ - if (getuid() == 0) { - const char *sockdirname = LOCAL_STATE_DIR "/run/libvirt"; - - if (chown(sockdirname, -1, unix_sock_gid) < 0) - VIR_ERROR(_("Failed to change group ownership of %s"), sockdirname); + if (unix_sock_dir && geteuid() == 0) { + if (chown(unix_sock_dir, -1, unix_sock_gid) < 0) + VIR_ERROR(_("Failed to change group ownership of %s"), + unix_sock_dir); } if (virEventAddHandleImpl(sigpipe[0], -- 1.6.1.155.g1b01da
From a95478864c4d81a952082e95729ce2476aa4ea64 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 8 Jan 2009 20:18:17 +0100 Subject: [PATCH 2/2] add two tests
* tests/libvirtd-pool: New file. Exercise the new unix_sock_dir option * tests/libvirtd-fail: New file. * tests/Makefile.am (test_scripts): Add omitted backslash, so that the virsh-all test is run. Add libvirtd-fail and libvirtd-pool. --- tests/Makefile.am | 24 ++++++++++--------- tests/libvirtd-fail | 20 ++++++++++++++++ tests/libvirtd-pool | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 11 deletions(-) create mode 100755 tests/libvirtd-fail create mode 100755 tests/libvirtd-pool diff --git a/tests/Makefile.am b/tests/Makefile.am index 0486ee3..c735d62 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -57,17 +57,19 @@ endif test_scripts = domainschematest if WITH_LIBVIRTD -test_scripts += \ - test_conf.sh \ - cpuset \ - daemon-conf \ - int-overflow \ - read-bufsiz \ - read-non-seekable \ - start \ - undefine \ - vcpupin - virsh-all +test_scripts += \ + cpuset \ + daemon-conf \ + int-overflow \ + libvirtd-fail \ + libvirtd-pool \ + read-bufsiz \ + read-non-seekable \ + start \ + test_conf.sh \ + undefine \ + vcpupin \ + virsh-all \ virsh-synopsis endif diff --git a/tests/libvirtd-fail b/tests/libvirtd-fail new file mode 100755 index 0000000..724d7c6 --- /dev/null +++ b/tests/libvirtd-fail @@ -0,0 +1,20 @@ +#!/bin/sh +# Ensure that libvirt fails when given nonexistent --config=FILE + +if test "$VERBOSE" = yes; then + set -x + libvirtd --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +libvirtd --config=no-such-file > log 2>&1 && fail=1 +cat <<\EOF > exp +Failed to open file 'no-such-file': No such file or directory +EOF + +compare exp log diff --git a/tests/libvirtd-pool b/tests/libvirtd-pool new file mode 100755 index 0000000..37bff3b --- /dev/null +++ b/tests/libvirtd-pool @@ -0,0 +1,62 @@ +#!/bin/sh +# Get coverage of libvirtd's config-parsing code. + +if test "$VERBOSE" = yes; then + set -x + libvirtd --version + virsh --version +fi + +test -z "$srcdir" && srcdir=$(pwd) +test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/.. +. "$srcdir/test-lib.sh" + +fail=0 + +pwd=$(pwd) || fail=1 +sock_dir="$pwd" +cat > conf <<EOF || fail=1 +unix_sock_dir = "$sock_dir" +EOF + +libvirtd --config=conf > libvirtd-log 2>&1 & pid=$! +sleep 1 + +url="qemu:///session?socket=@$sock_dir/libvirt-sock" +virsh --connect "$url" \ + pool-define-as P dir src-host /src/path /src/dev S /target-path > out 2>&1 \ + || fail=1 +virsh --connect "$url" pool-dumpxml P >> out 2>&1 || fail=1 + +# remove random uuid +sed 's/<uuid>.*/-/' out > k && mv k out || fail=1 + +kill $pid + +cat <<EOF > pool-list-exp +Pool P defined + +<pool type='dir'> + <name>P</name> + - + <capacity>0</capacity> + <allocation>0</allocation> + <available>0</available> + <source> + </source> + <target> + <path>/target-path</path> + <permissions> + <mode>0700</mode> + <owner>500</owner> + <group>500</group> + </permissions> + </target> +</pool> + +EOF + +compare pool-list-exp out || fail=1 +compare /dev/null libvirtd-log || fail=1 + +exit $fail -- 1.6.1.155.g1b01da
Jim Meyering <jim@meyering.net> wrote:
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
The QEMU driver runs as non-root too. This is what the qemu:///session URI is used for. Likewise with the UML driver. The existing tests that invoke libvirtd fail quite frequently for me already due to them activating the QEMU / UML drivers. We really need a way to explicitly say what drivers should be allowed by the daemon, overriding what's compiled in. THis could in fact be useful even for production deployment, allowing site admins to guarentee that Xen driver is never used in the daemon even if it is compiled in by default.
So perhaps a couple of config params like
allowed_drivers = [ "qemu", "xen", "test" ] unix_sock_dir = "/var/run/libvirt/"
Not sure how best to hook the first one up to libvirt.so though - the virInitialize/virStateInitize calls always activate all of them, with no easy way to disable.
Sounds good. I'm deferring "allowed_drivers" for now, and preparing a patch to add support for a new configuration parameter
unix_sock_dir
and also for
log_dir
I'll add only unix_sock_dir for starters:
The first patch adds the new parameter, and the second uses it in one of the two new libvirtd-running tests.
Here's one more change, this time to make the final actual libvirtd-running test use the new parameter:
From 337b40846c8510e00b33d6ba8bedb045d99bd721 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 13 Jan 2009 10:54:41 +0100 Subject: [PATCH] * tests/daemon-conf: Specify a non-default socket directory.
--- tests/daemon-conf | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/tests/daemon-conf b/tests/daemon-conf index b357c39..55671b8 100755 --- a/tests/daemon-conf +++ b/tests/daemon-conf @@ -71,6 +71,11 @@ done # Run with the unmodified config file. sleep_secs=2 + +# Be careful to specify a non-default socket directory: +sed 's,^unix_sock_dir.*,unix_sock_dir="'"$(pwd)"'",' tmp.conf > k || fail=1 +mv k tmp.conf || fail=1 + printf "running libvirtd with a valid config file ($sleep_secs seconds)\n" 1>&2 libvirtd --config=tmp.conf > log 2>&1 & pid=$! sleep $sleep_secs -- 1.6.1.198.g1eb4d
Jim Meyering <jim@meyering.net> wrote:
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
The QEMU driver runs as non-root too. This is what the qemu:///session URI is used for. Likewise with the UML driver. The existing tests that invoke libvirtd fail quite frequently for me already due to them activating the QEMU / UML drivers. We really need a way to explicitly say what drivers should be allowed by the daemon, overriding what's compiled in. THis could in fact be useful even for production deployment, allowing site admins to guarentee that Xen driver is never used in the daemon even if it is compiled in by default.
So perhaps a couple of config params like
allowed_drivers = [ "qemu", "xen", "test" ] unix_sock_dir = "/var/run/libvirt/"
Not sure how best to hook the first one up to libvirt.so though - the virInitialize/virStateInitize calls always activate all of them, with no easy way to disable.
Sounds good. I'm deferring "allowed_drivers" for now, and preparing a patch to add support for a new configuration parameter
unix_sock_dir
and also for
log_dir
I'll add only unix_sock_dir for starters:
I've rebased this and made some minor improvements, like calling virReportOOMError and having a single exit point.
From 907671319b056495eef1d146dc9260a1a2fcb64c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 12 Jan 2009 17:17:19 +0100 Subject: [PATCH] libvirtd: new config-file option: unix_sock_dir
Before this change, the unix socket directory was hard-coded to be e.g., /var/run/libvirt for euid==0 and ~/.libvirt otherwise. With this change, you may now specify that directory in libvirtd's config file via a line like this: unix_sock_dir = "/var/run/libvirt". This is essential for running tests that do not impinge on any existing libvirtd process, and in running tests in parallel. * qemud/libvirtd.conf (unix_sock_dir): Add comment and example. * qemud/qemud.c (unix_sock_dir): New global (remoteReadConfigFile): Set the global. (qemudInitPaths): Use the global, unix_sock_dir, if non-NULL. One minor improvement: unlink both sockets or none, never just one of them. (main): Use the new global rather than hard-coding "/run/libvirt". * qemud/libvirtd.aug (sock_acl_entry): Add "unix_sock_dir". --- qemud/libvirtd.aug | 2 +- qemud/libvirtd.conf | 3 +- qemud/qemud.c | 98 +++++++++++++++++++++++++++++++++------------------ 3 files changed, 67 insertions(+), 36 deletions(-) diff --git a/qemud/libvirtd.aug b/qemud/libvirtd.aug index 40acd93..7406d23 100644 --- a/qemud/libvirtd.aug +++ b/qemud/libvirtd.aug @@ -35,6 +35,7 @@ module Libvirtd = let sock_acl_entry = str_entry "unix_sock_group" | str_entry "unix_sock_ro_perms" | str_entry "unix_sock_rw_perms" + | str_entry "unix_sock_dir" let authentication_entry = str_entry "auth_unix_ro" | str_entry "auth_unix_rw" @@ -79,4 +80,3 @@ module Libvirtd = . Util.stdexcl let xfm = transform lns filter - diff --git a/qemud/libvirtd.conf b/qemud/libvirtd.conf index 4932084..1fd5918 100644 --- a/qemud/libvirtd.conf +++ b/qemud/libvirtd.conf @@ -97,7 +97,8 @@ # control then you may want to relax this to: #unix_sock_rw_perms = "0770" - +# Set the name of the directory in which sockets will be found/created. +#unix_sock_dir = "/var/run/libvirt" ################################################################# # diff --git a/qemud/qemud.c b/qemud/qemud.c index a4add5a..f8c3c97 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -51,6 +51,8 @@ #include "libvirt_internal.h" #include "virterror_internal.h" +#define VIR_FROM_THIS VIR_FROM_QEMU + #include "qemud.h" #include "util.h" #include "remote_internal.h" @@ -136,6 +138,8 @@ static char *listen_addr = (char *) LIBVIRTD_LISTEN_ADDR; static char *tls_port = (char *) LIBVIRTD_TLS_PORT; static char *tcp_port = (char *) LIBVIRTD_TCP_PORT; +static char *unix_sock_dir = NULL; + #if HAVE_POLKIT static int auth_unix_rw = REMOTE_AUTH_POLKIT; static int auth_unix_ro = REMOTE_AUTH_POLKIT; @@ -712,46 +716,71 @@ static int qemudInitPaths(struct qemud_server *server, int maxlen) { uid_t uid = geteuid(); + char *sock_dir; + char *dir_prefix = NULL; + int ret = -1; + char *sock_dir_prefix = NULL; + + if (unix_sock_dir) + sock_dir = unix_sock_dir; + else { + sock_dir = sockname; + if (uid == SYSTEM_UID) { + dir_prefix = strdup (LOCAL_STATE_DIR); + if (dir_prefix == NULL) { + virReportOOMError(NULL); + goto cleanup; + } + if (snprintf (sock_dir, maxlen, "%s/run/libvirt", + dir_prefix) >= maxlen) + goto snprintf_error; + } else { + dir_prefix = virGetUserDirectory(NULL, uid); + if (dir_prefix == NULL) { + /* Do not diagnose here; virGetUserDirectory does that. */ + goto snprintf_error; + } - if (uid == SYSTEM_UID) { - if (snprintf (sockname, maxlen, "%s/run/libvirt/libvirt-sock", - LOCAL_STATE_DIR) >= maxlen) - goto snprintf_error; + if (snprintf(sock_dir, maxlen, "%s/.libvirt", dir_prefix) >= maxlen) + goto snprintf_error; + } + } - unlink(sockname); + sock_dir_prefix = strdup (sock_dir); + if (!sock_dir_prefix) { + virReportOOMError(NULL); + goto cleanup; + } - if (snprintf (roSockname, maxlen, "%s/run/libvirt/libvirt-sock-ro", - LOCAL_STATE_DIR) >= maxlen) + if (uid == SYSTEM_UID) { + if (snprintf (sockname, maxlen, "%s/libvirt-sock", + sock_dir_prefix) >= maxlen + || (snprintf (roSockname, maxlen, "%s/libvirt-sock-ro", + sock_dir_prefix) >= maxlen)) goto snprintf_error; - + unlink(sockname); unlink(roSockname); - - if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", LOCAL_STATE_DIR) >= PATH_MAX) - goto snprintf_error; } else { - char *userdir = virGetUserDirectory(NULL, uid); - if (userdir == NULL) { - /* Do not diagnose here; virGetUserDirectory does that. */ - return -1; - } - - if (snprintf(sockname, maxlen, "@%s/.libvirt/libvirt-sock", userdir) >= maxlen) { - VIR_FREE(userdir); + if (snprintf(sockname, maxlen, "@%s/libvirt-sock", + sock_dir_prefix) >= maxlen) goto snprintf_error; - } + } - if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", userdir) >= PATH_MAX) { - VIR_FREE(userdir); - goto snprintf_error; - } - VIR_FREE(userdir); - } /* !remote */ + if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", + dir_prefix) >= PATH_MAX) + goto snprintf_error; - return 0; + ret = 0; snprintf_error: - VIR_ERROR("%s", _("Resulting path too long for buffer in qemudInitPaths()")); - return -1; + if (ret) + VIR_ERROR("%s", + _("Resulting path too long for buffer in qemudInitPaths()")); + + cleanup: + free (dir_prefix); + free (sock_dir_prefix); + return ret; } static struct qemud_server *qemudInitialize(int sigread) { @@ -2556,6 +2585,8 @@ remoteReadConfigFile (struct qemud_server *server, const char *filename) unix_sock_rw_perms = NULL; } + GET_CONF_STR (conf, filename, unix_sock_dir); + GET_CONF_INT (conf, filename, mdns_adv); GET_CONF_STR (conf, filename, mdns_name); @@ -2846,11 +2877,10 @@ int main(int argc, char **argv) { goto error2; /* Change the group ownership of /var/run/libvirt to unix_sock_gid */ - if (getuid() == 0) { - const char *sockdirname = LOCAL_STATE_DIR "/run/libvirt"; - - if (chown(sockdirname, -1, unix_sock_gid) < 0) - VIR_ERROR(_("Failed to change group ownership of %s"), sockdirname); + if (unix_sock_dir && geteuid() == 0) { + if (chown(unix_sock_dir, -1, unix_sock_gid) < 0) + VIR_ERROR(_("Failed to change group ownership of %s"), + unix_sock_dir); } if (virEventAddHandleImpl(sigpipe[0], -- 1.6.2.rc0.173.g5e148
On Mon, Feb 09, 2009 at 11:25:24AM +0100, Jim Meyering wrote:
I've rebased this and made some minor improvements, like calling virReportOOMError and having a single exit point.
From 907671319b056495eef1d146dc9260a1a2fcb64c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 12 Jan 2009 17:17:19 +0100 Subject: [PATCH] libvirtd: new config-file option: unix_sock_dir
Before this change, the unix socket directory was hard-coded to be e.g., /var/run/libvirt for euid==0 and ~/.libvirt otherwise. With this change, you may now specify that directory in libvirtd's config file via a line like this: unix_sock_dir = "/var/run/libvirt". This is essential for running tests that do not impinge on any existing libvirtd process, and in running tests in parallel. * qemud/libvirtd.conf (unix_sock_dir): Add comment and example. * qemud/qemud.c (unix_sock_dir): New global (remoteReadConfigFile): Set the global. (qemudInitPaths): Use the global, unix_sock_dir, if non-NULL. One minor improvement: unlink both sockets or none, never just one of them. (main): Use the new global rather than hard-coding "/run/libvirt". * qemud/libvirtd.aug (sock_acl_entry): Add "unix_sock_dir". --- qemud/libvirtd.aug | 2 +- qemud/libvirtd.conf | 3 +- qemud/qemud.c | 98 +++++++++++++++++++++++++++++++++------------------ 3 files changed, 67 insertions(+), 36 deletions(-)
diff --git a/qemud/libvirtd.aug b/qemud/libvirtd.aug index 40acd93..7406d23 100644 --- a/qemud/libvirtd.aug +++ b/qemud/libvirtd.aug @@ -35,6 +35,7 @@ module Libvirtd = let sock_acl_entry = str_entry "unix_sock_group" | str_entry "unix_sock_ro_perms" | str_entry "unix_sock_rw_perms" + | str_entry "unix_sock_dir"
let authentication_entry = str_entry "auth_unix_ro" | str_entry "auth_unix_rw" @@ -79,4 +80,3 @@ module Libvirtd = . Util.stdexcl
let xfm = transform lns filter - diff --git a/qemud/libvirtd.conf b/qemud/libvirtd.conf index 4932084..1fd5918 100644 --- a/qemud/libvirtd.conf +++ b/qemud/libvirtd.conf @@ -97,7 +97,8 @@ # control then you may want to relax this to: #unix_sock_rw_perms = "0770"
- +# Set the name of the directory in which sockets will be found/created. +#unix_sock_dir = "/var/run/libvirt"
################################################################# # diff --git a/qemud/qemud.c b/qemud/qemud.c index a4add5a..f8c3c97 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -51,6 +51,8 @@ #include "libvirt_internal.h" #include "virterror_internal.h"
+#define VIR_FROM_THIS VIR_FROM_QEMU + #include "qemud.h" #include "util.h" #include "remote_internal.h" @@ -136,6 +138,8 @@ static char *listen_addr = (char *) LIBVIRTD_LISTEN_ADDR; static char *tls_port = (char *) LIBVIRTD_TLS_PORT; static char *tcp_port = (char *) LIBVIRTD_TCP_PORT;
+static char *unix_sock_dir = NULL; + #if HAVE_POLKIT static int auth_unix_rw = REMOTE_AUTH_POLKIT; static int auth_unix_ro = REMOTE_AUTH_POLKIT; @@ -712,46 +716,71 @@ static int qemudInitPaths(struct qemud_server *server, int maxlen) { uid_t uid = geteuid(); + char *sock_dir; + char *dir_prefix = NULL; + int ret = -1; + char *sock_dir_prefix = NULL; + + if (unix_sock_dir) + sock_dir = unix_sock_dir; + else { + sock_dir = sockname; + if (uid == SYSTEM_UID) { + dir_prefix = strdup (LOCAL_STATE_DIR); + if (dir_prefix == NULL) { + virReportOOMError(NULL); + goto cleanup; + } + if (snprintf (sock_dir, maxlen, "%s/run/libvirt", + dir_prefix) >= maxlen) + goto snprintf_error; + } else { + dir_prefix = virGetUserDirectory(NULL, uid); + if (dir_prefix == NULL) { + /* Do not diagnose here; virGetUserDirectory does that. */ + goto snprintf_error; + }
[snip]
+ if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", + dir_prefix) >= PATH_MAX) + goto snprintf_error;
If I'm reading correctly, this will cause system logs to get put in the directory /var/.libvirt/log instead of /var/log/libvirt, since this snprintf doesn't take account of uid == SYSTEM_UID as the old code used todo. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
[snip]
+ if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", + dir_prefix) >= PATH_MAX) + goto snprintf_error;
If I'm reading correctly, this will cause system logs to get put in the directory /var/.libvirt/log instead of /var/log/libvirt, since this snprintf doesn't take account of uid == SYSTEM_UID as the old code used todo.
Good catch. I'd offer to add a root-only test to make sure the log file is created as advertised, but would rather first add another config-file option: to specify where the log file goes. However, first things first: Here's a patch that adds two blocks, neither pretty, but with less duplication than the 3rd alternative, which duplicates both the snprintf and the result comparison. (of course, I'll use only one of them) BTW, this also eliminates the uses of PATH_MAX that were vestiges of a messy rebase. Now we test against maxlen. Of these two, I prefer the latter (slightly less duplication). Do you care? diff --git a/qemud/qemud.c b/qemud/qemud.c index f8c3c97..ddcb6ff 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -766,8 +766,16 @@ static int qemudInitPaths(struct qemud_server *server, goto snprintf_error; } - if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", - dir_prefix) >= PATH_MAX) + if ((uid == SYSTEM_UID + ? snprintf(server->logDir, maxlen, "%s/log/libvirt", LOCAL_STATE_DIR) + : snprintf(server->logDir, maxlen, "%s/.libvirt/log", dir_prefix)) + >= maxlen) + goto snprintf_error; + + if (snprintf(server->logDir, maxlen, + (uid == SYSTEM_UID ? "%s/log/libvirt" : "%s/.libvirt/log"), + (uid == SYSTEM_UID ? LOCAL_STATE_DIR : dir_prefix)) + >= maxlen) goto snprintf_error; ret = 0;
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
[snip]
+ if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", + dir_prefix) >= PATH_MAX) + goto snprintf_error;
If I'm reading correctly, this will cause system logs to get put in the directory /var/.libvirt/log instead of /var/log/libvirt, since this snprintf doesn't take account of uid == SYSTEM_UID as the old code used todo.
Good catch. I'd offer to add a root-only test to make sure the log file is created as advertised, but would rather first add another config-file option: to specify where the log file goes.
I just remembered: the log-file configuration is already possible. Just put something like this in the config file: log_outputs="3:file:/path-to-dir/log"
On Mon, Feb 09, 2009 at 02:01:19PM +0100, Jim Meyering wrote:
However, first things first:
Here's a patch that adds two blocks, neither pretty, but with less duplication than the 3rd alternative, which duplicates both the snprintf and the result comparison. (of course, I'll use only one of them)
BTW, this also eliminates the uses of PATH_MAX that were vestiges of a messy rebase. Now we test against maxlen.
Of these two, I prefer the latter (slightly less duplication). Do you care?
I don't particular like either option - too much line noise in there. Two thoughts - printf is redundant in the first place, since the compiler will happily concatenate 2 static strings, so we can just strdup(). In the second case, we could asprintf instead, and so have something like if (uid == SYSTEM_UID) server->logDir = strdup(LOCAL_STATE_DIR "/log/libvirt") else virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix)) if (!server->logDir) ... oom handling ...
diff --git a/qemud/qemud.c b/qemud/qemud.c index f8c3c97..ddcb6ff 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -766,8 +766,16 @@ static int qemudInitPaths(struct qemud_server *server, goto snprintf_error; }
- if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", - dir_prefix) >= PATH_MAX) + if ((uid == SYSTEM_UID + ? snprintf(server->logDir, maxlen, "%s/log/libvirt", LOCAL_STATE_DIR) + : snprintf(server->logDir, maxlen, "%s/.libvirt/log", dir_prefix)) + >= maxlen) + goto snprintf_error; + + if (snprintf(server->logDir, maxlen, + (uid == SYSTEM_UID ? "%s/log/libvirt" : "%s/.libvirt/log"), + (uid == SYSTEM_UID ? LOCAL_STATE_DIR : dir_prefix)) + >= maxlen) goto snprintf_error;
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Feb 09, 2009 at 02:01:19PM +0100, Jim Meyering wrote:
However, first things first:
Here's a patch that adds two blocks, neither pretty, but with less duplication than the 3rd alternative, which duplicates both the snprintf and the result comparison. (of course, I'll use only one of them)
BTW, this also eliminates the uses of PATH_MAX that were vestiges of a messy rebase. Now we test against maxlen.
Of these two, I prefer the latter (slightly less duplication). Do you care?
I don't particular like either option - too much line noise in there. Two thoughts - printf is redundant in the first place, since the compiler will happily concatenate 2 static strings, so we can just strdup(). In the second case, we could asprintf instead, and so have something like
if (uid == SYSTEM_UID) server->logDir = strdup(LOCAL_STATE_DIR "/log/libvirt") else virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix))
if (!server->logDir) ... oom handling ...
Can't do that. The logDir member is declared like this: qemud/qemud.h: char logDir[PATH_MAX]; which made me see why it was using PATH_MAX. and we always want to check for buffer overrun. so here's what I'll do: diff --git a/qemud/qemud.c b/qemud/qemud.c index f8c3c97..d9d7ce3 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -766,8 +766,10 @@ static int qemudInitPaths(struct qemud_server *server, goto snprintf_error; } - if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", - dir_prefix) >= PATH_MAX) + if (snprintf(server->logDir, sizeof (server->logDir), + (uid == SYSTEM_UID ? "%s/log/libvirt" : "%s/.libvirt/log"), + (uid == SYSTEM_UID ? LOCAL_STATE_DIR : dir_prefix)) + >= sizeof (server->logDir)) goto snprintf_error; ret = 0;
On Mon, Feb 09, 2009 at 03:39:15PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Feb 09, 2009 at 02:01:19PM +0100, Jim Meyering wrote:
However, first things first:
Here's a patch that adds two blocks, neither pretty, but with less duplication than the 3rd alternative, which duplicates both the snprintf and the result comparison. (of course, I'll use only one of them)
BTW, this also eliminates the uses of PATH_MAX that were vestiges of a messy rebase. Now we test against maxlen.
Of these two, I prefer the latter (slightly less duplication). Do you care?
I don't particular like either option - too much line noise in there. Two thoughts - printf is redundant in the first place, since the compiler will happily concatenate 2 static strings, so we can just strdup(). In the second case, we could asprintf instead, and so have something like
if (uid == SYSTEM_UID) server->logDir = strdup(LOCAL_STATE_DIR "/log/libvirt") else virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix))
if (!server->logDir) ... oom handling ...
Can't do that. The logDir member is declared like this:
qemud/qemud.h: char logDir[PATH_MAX];
No, I meant change that to just 'char *logDir' since pre-declaring it PATH_MAX isn't really helping to simplify the code at all. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Feb 09, 2009 at 03:39:15PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Mon, Feb 09, 2009 at 02:01:19PM +0100, Jim Meyering wrote:
However, first things first:
Here's a patch that adds two blocks, neither pretty, but with less duplication than the 3rd alternative, which duplicates both the snprintf and the result comparison. (of course, I'll use only one of them)
BTW, this also eliminates the uses of PATH_MAX that were vestiges of a messy rebase. Now we test against maxlen.
Of these two, I prefer the latter (slightly less duplication). Do you care?
I don't particular like either option - too much line noise in there. Two thoughts - printf is redundant in the first place, since the compiler
Um. checking snprintf's result was not redundant.
will happily concatenate 2 static strings, so we can just strdup(). In the second case, we could asprintf instead, and so have something like
if (uid == SYSTEM_UID) server->logDir = strdup(LOCAL_STATE_DIR "/log/libvirt") else virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix))
if (!server->logDir) ... oom handling ...
Can't do that. The logDir member is declared like this:
qemud/qemud.h: char logDir[PATH_MAX];
No, I meant change that to just 'char *logDir' since pre-declaring it PATH_MAX isn't really helping to simplify the code at all.
In that case, no problem. In that context, your saying printf is redundant makes sense ;-) Will do. Besides, that's more consistent with what's done e.g., in src/lxc_conf.h: char *logDir; src/qemu_conf.h: char *logDir; src/uml_conf.h: char *logDir;
Here's the result: FYI: additional incremental changes were: * qemud/qemud.h (struct qemud_server) [logDir]: Change type from char[PATH_MAX] to char*. * qemud/qemud.c: Adjust accordingly. (qemudCleanup): Free logDir. Here's the full patch:
From 367b5f9932afb5fac14837481ae9ea4f6187e8d4 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 12 Jan 2009 17:17:19 +0100 Subject: [PATCH] libvirtd: new config-file option: unix_sock_dir
Before this change, the unix socket directory was hard-coded to be e.g., /var/run/libvirt for euid==0 and ~/.libvirt otherwise. With this change, you may now specify that directory in libvirtd's config file via a line like this: unix_sock_dir = "/var/run/libvirt". This is essential for running tests that do not impinge on any existing libvirtd process, and in running tests in parallel. * qemud/libvirtd.conf (unix_sock_dir): Add comment and example. * qemud/qemud.h (struct qemud_server) [logDir]: Change type from char[PATH_MAX] to char*. * qemud/qemud.c (unix_sock_dir): New global (remoteReadConfigFile): Set the global. (qemudInitPaths): Use the global, unix_sock_dir, if non-NULL. One minor improvement: unlink both sockets or none, never just one of them. (qemudCleanup): Free logDir. (main): Use the new global rather than hard-coding "/run/libvirt". * qemud/libvirtd.aug (sock_acl_entry): Add "unix_sock_dir". --- qemud/libvirtd.aug | 2 +- qemud/libvirtd.conf | 3 +- qemud/qemud.c | 103 ++++++++++++++++++++++++++++++++++----------------- qemud/qemud.h | 4 +- 4 files changed, 74 insertions(+), 38 deletions(-) diff --git a/qemud/libvirtd.aug b/qemud/libvirtd.aug index 40acd93..7406d23 100644 --- a/qemud/libvirtd.aug +++ b/qemud/libvirtd.aug @@ -35,6 +35,7 @@ module Libvirtd = let sock_acl_entry = str_entry "unix_sock_group" | str_entry "unix_sock_ro_perms" | str_entry "unix_sock_rw_perms" + | str_entry "unix_sock_dir" let authentication_entry = str_entry "auth_unix_ro" | str_entry "auth_unix_rw" @@ -79,4 +80,3 @@ module Libvirtd = . Util.stdexcl let xfm = transform lns filter - diff --git a/qemud/libvirtd.conf b/qemud/libvirtd.conf index 4932084..1fd5918 100644 --- a/qemud/libvirtd.conf +++ b/qemud/libvirtd.conf @@ -97,7 +97,8 @@ # control then you may want to relax this to: #unix_sock_rw_perms = "0770" - +# Set the name of the directory in which sockets will be found/created. +#unix_sock_dir = "/var/run/libvirt" ################################################################# # diff --git a/qemud/qemud.c b/qemud/qemud.c index a4add5a..ca6357c 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -51,6 +51,8 @@ #include "libvirt_internal.h" #include "virterror_internal.h" +#define VIR_FROM_THIS VIR_FROM_QEMU + #include "qemud.h" #include "util.h" #include "remote_internal.h" @@ -136,6 +138,8 @@ static char *listen_addr = (char *) LIBVIRTD_LISTEN_ADDR; static char *tls_port = (char *) LIBVIRTD_TLS_PORT; static char *tcp_port = (char *) LIBVIRTD_TCP_PORT; +static char *unix_sock_dir = NULL; + #if HAVE_POLKIT static int auth_unix_rw = REMOTE_AUTH_POLKIT; static int auth_unix_ro = REMOTE_AUTH_POLKIT; @@ -712,46 +716,75 @@ static int qemudInitPaths(struct qemud_server *server, int maxlen) { uid_t uid = geteuid(); + char *sock_dir; + char *dir_prefix = NULL; + int ret = -1; + char *sock_dir_prefix = NULL; + + if (unix_sock_dir) + sock_dir = unix_sock_dir; + else { + sock_dir = sockname; + if (uid == SYSTEM_UID) { + dir_prefix = strdup (LOCAL_STATE_DIR); + if (dir_prefix == NULL) { + virReportOOMError(NULL); + goto cleanup; + } + if (snprintf (sock_dir, maxlen, "%s/run/libvirt", + dir_prefix) >= maxlen) + goto snprintf_error; + } else { + dir_prefix = virGetUserDirectory(NULL, uid); + if (dir_prefix == NULL) { + /* Do not diagnose here; virGetUserDirectory does that. */ + goto snprintf_error; + } - if (uid == SYSTEM_UID) { - if (snprintf (sockname, maxlen, "%s/run/libvirt/libvirt-sock", - LOCAL_STATE_DIR) >= maxlen) - goto snprintf_error; + if (snprintf(sock_dir, maxlen, "%s/.libvirt", dir_prefix) >= maxlen) + goto snprintf_error; + } + } - unlink(sockname); + sock_dir_prefix = strdup (sock_dir); + if (!sock_dir_prefix) { + virReportOOMError(NULL); + goto cleanup; + } - if (snprintf (roSockname, maxlen, "%s/run/libvirt/libvirt-sock-ro", - LOCAL_STATE_DIR) >= maxlen) + if (uid == SYSTEM_UID) { + if (snprintf (sockname, maxlen, "%s/libvirt-sock", + sock_dir_prefix) >= maxlen + || (snprintf (roSockname, maxlen, "%s/libvirt-sock-ro", + sock_dir_prefix) >= maxlen)) goto snprintf_error; - + unlink(sockname); unlink(roSockname); - - if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", LOCAL_STATE_DIR) >= PATH_MAX) - goto snprintf_error; } else { - char *userdir = virGetUserDirectory(NULL, uid); - if (userdir == NULL) { - /* Do not diagnose here; virGetUserDirectory does that. */ - return -1; - } - - if (snprintf(sockname, maxlen, "@%s/.libvirt/libvirt-sock", userdir) >= maxlen) { - VIR_FREE(userdir); + if (snprintf(sockname, maxlen, "@%s/libvirt-sock", + sock_dir_prefix) >= maxlen) goto snprintf_error; - } + } - if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log", userdir) >= PATH_MAX) { - VIR_FREE(userdir); - goto snprintf_error; - } - VIR_FREE(userdir); - } /* !remote */ + if (uid == SYSTEM_UID) + server->logDir = strdup (LOCAL_STATE_DIR "/log/libvirt"); + else + virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix); - return 0; + if (server->logDir == NULL) + virReportOOMError(NULL); + + ret = 0; snprintf_error: - VIR_ERROR("%s", _("Resulting path too long for buffer in qemudInitPaths()")); - return -1; + if (ret) + VIR_ERROR("%s", + _("Resulting path too long for buffer in qemudInitPaths()")); + + cleanup: + free (dir_prefix); + free (sock_dir_prefix); + return ret; } static struct qemud_server *qemudInitialize(int sigread) { @@ -2208,6 +2241,7 @@ static void qemudCleanup(struct qemud_server *server) { free(sock); sock = next; } + free(server->logDir); #ifdef HAVE_SASL if (server->saslUsernameWhitelist) { @@ -2556,6 +2590,8 @@ remoteReadConfigFile (struct qemud_server *server, const char *filename) unix_sock_rw_perms = NULL; } + GET_CONF_STR (conf, filename, unix_sock_dir); + GET_CONF_INT (conf, filename, mdns_adv); GET_CONF_STR (conf, filename, mdns_name); @@ -2846,11 +2882,10 @@ int main(int argc, char **argv) { goto error2; /* Change the group ownership of /var/run/libvirt to unix_sock_gid */ - if (getuid() == 0) { - const char *sockdirname = LOCAL_STATE_DIR "/run/libvirt"; - - if (chown(sockdirname, -1, unix_sock_gid) < 0) - VIR_ERROR(_("Failed to change group ownership of %s"), sockdirname); + if (unix_sock_dir && geteuid() == 0) { + if (chown(unix_sock_dir, -1, unix_sock_gid) < 0) + VIR_ERROR(_("Failed to change group ownership of %s"), + unix_sock_dir); } if (virEventAddHandleImpl(sigpipe[0], diff --git a/qemud/qemud.h b/qemud/qemud.h index 7938f89..4ddbc41 100644 --- a/qemud/qemud.h +++ b/qemud/qemud.h @@ -1,7 +1,7 @@ /* * qemud.h: daemon data structure definitions * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -181,7 +181,7 @@ struct qemud_server { struct qemud_client **clients; int sigread; - char logDir[PATH_MAX]; + char *logDir; unsigned int shutdown : 1; #ifdef HAVE_AVAHI struct libvirtd_mdns *mdns; -- 1.6.2.rc0.173.g5e148
On Mon, Jan 12, 2009 at 01:28:41PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
+ if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + }
That should be R_OK, not X_OK - the config files aren't marked executable, so this will never load the config. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel Berrange <berrange@redhat.com> wrote:
On Mon, Jan 12, 2009 at 01:28:41PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
+ if (remote_config_file == NULL) { + static const char *default_config_file + = SYSCONF_DIR "/libvirt/libvirtd.conf"; + remote_config_file = + (access(default_config_file, X_OK) == 0 + ? default_config_file + : "/dev/null"); + }
That should be R_OK, not X_OK - the config files aren't marked executable, so this will never load the config.
Right you are. I've just committed this:
From 76eb37b44dd3e51e6ab5304c4e57c10be1ff53a5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 13 Jan 2009 13:20:54 +0100 Subject: [PATCH] qemud.c: fix error in yesterday's change: s/X_OK/R_OK/
* qemud/qemud.c (main): Fix error s/X_OK/R_OK/ reported by Daniel Berrange. --- qemud/qemud.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index 336f0a9..6a2db78 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -2371,7 +2371,7 @@ int main(int argc, char **argv) { static const char *default_config_file = SYSCONF_DIR "/libvirt/libvirtd.conf"; remote_config_file = - (access(default_config_file, X_OK) == 0 + (access(default_config_file, R_OK) == 0 ? default_config_file : "/dev/null"); } -- 1.6.1.155.g1b01da
participants (3)
-
Daniel Berrange -
Daniel P. Berrange
-
Jim Meyering