[libvirt] [PATCH 1/5] write pid file into stateDir
Let qemu write out a pid file so we can check in stateDir for running vm later. TODO: Fix up the rest of the qemuxml2argvdata testcases, simple search and replace. I sikpped that for now since it makes rebasing harder. --- src/libvirt_sym.version.in | 1 + src/qemu_conf.c | 7 +++++++ src/qemu_conf.h | 1 + src/qemu_driver.c | 15 +++++++++++++++ src/util.c | 20 ++++++++++++++++---- src/util.h | 2 ++ .../qemuxml2argv-hostdev-usb-address.args | 2 +- tests/qemuxml2argvtest.c | 2 ++ 8 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/libvirt_sym.version.in b/src/libvirt_sym.version.in index de0bc4a..1eff790 100644 --- a/src/libvirt_sym.version.in +++ b/src/libvirt_sym.version.in @@ -592,6 +592,7 @@ LIBVIRT_PRIVATE_@VERSION@ { virFileMakePath; virFileOpenTty; virFileReadLimFD; + virFilePid; virFileReadPid; virFileLinkPointsTo; virParseNumber; diff --git a/src/qemu_conf.c b/src/qemu_conf.c index e6c378f..218aefa 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -724,6 +724,7 @@ int qemudBuildCommandLine(virConnectPtr conn, const char *emulator; char uuid[VIR_UUID_STRING_BUFLEN]; char domid[50]; + char *pidfile; uname(&ut); @@ -816,6 +817,9 @@ int qemudBuildCommandLine(virConnectPtr conn, snprintf(memory, sizeof(memory), "%lu", vm->def->memory/1024); snprintf(vcpus, sizeof(vcpus), "%lu", vm->def->vcpus); snprintf(domid, sizeof(domid), "%d", vm->def->id); + pidfile = virFilePid(driver->stateDir, vm->def->name); + if (!pidfile) + goto error; ADD_ENV_LIT("LC_ALL=C"); @@ -870,6 +874,9 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT("-monitor"); ADD_ARG_LIT("pty"); + ADD_ARG_LIT("-pidfile"); + ADD_ARG(pidfile); + if (vm->def->localtime) ADD_ARG_LIT("-localtime"); diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 36d09d1..ffbd0e7 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -62,6 +62,7 @@ struct qemud_driver { char *configDir; char *autostartDir; char *logDir; + char *stateDir; unsigned int vncTLS : 1; unsigned int vncTLSx509verify : 1; char *vncTLSx509certdir; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ca96223..a2e573e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -220,6 +220,10 @@ qemudStartup(void) { if ((base = strdup (SYSCONF_DIR "/libvirt")) == NULL) goto out_of_memory; + + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", LOCAL_STATE_DIR) == -1) + goto out_of_memory; } else { if (!(pw = getpwuid(uid))) { qemudLog(QEMUD_ERR, _("Failed to find user record for uid '%d': %s\n"), @@ -233,6 +237,16 @@ qemudStartup(void) { if (asprintf (&base, "%s/.libvirt", pw->pw_dir) == -1) goto out_of_memory; + + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", base) == -1) + goto out_of_memory; + } + + if (virFileMakePath(qemu_driver->stateDir) < 0) { + qemudLog(QEMUD_ERR, _("Failed to create state dir '%s': %s\n"), + qemu_driver->stateDir, strerror(errno)); + goto error; } /* Configuration paths are either ~/.libvirt/qemu/... (session) or @@ -378,6 +392,7 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->logDir); VIR_FREE(qemu_driver->configDir); VIR_FREE(qemu_driver->autostartDir); + VIR_FREE(qemu_driver->stateDir); VIR_FREE(qemu_driver->vncTLSx509certdir); VIR_FREE(qemu_driver->vncListen); diff --git a/src/util.c b/src/util.c index da26009..481f136 100644 --- a/src/util.c +++ b/src/util.c @@ -918,6 +918,17 @@ int virFileOpenTty(int *ttymaster ATTRIBUTE_UNUSED, #endif +char* virFilePid(const char *dir, const char* name) +{ + char* pidfile; + + if (asprintf(&pidfile, "%s/%s.pid", dir, name) < 0) { + pidfile = NULL; + } + return pidfile; +} + + int virFileWritePid(const char *dir, const char *name, pid_t pid) @@ -930,7 +941,7 @@ int virFileWritePid(const char *dir, if ((rc = virFileMakePath(dir))) goto cleanup; - if (asprintf(&pidfile, "%s/%s.pid", dir, name) < 0) { + if (!(pidfile = virFilePid(dir, name))) { rc = ENOMEM; goto cleanup; } @@ -973,7 +984,8 @@ int virFileReadPid(const char *dir, FILE *file; char *pidfile = NULL; *pid = 0; - if (asprintf(&pidfile, "%s/%s.pid", dir, name) < 0) { + + if (!(pidfile = virFilePid(dir, name))) { rc = ENOMEM; goto cleanup; } @@ -1006,8 +1018,8 @@ int virFileDeletePid(const char *dir, int rc = 0; char *pidfile = NULL; - if (asprintf(&pidfile, "%s/%s.pid", dir, name) < 0) { - rc = errno; + if (!(pidfile = virFilePid(dir, name))) { + rc = ENOMEM; goto cleanup; } diff --git a/src/util.h b/src/util.h index 0748cbf..58488ae 100644 --- a/src/util.h +++ b/src/util.h @@ -79,6 +79,8 @@ int virFileOpenTty(int *ttymaster, char **ttyName, int rawmode); +char* virFilePid(const char *dir, + const char *name); int virFileWritePid(const char *dir, const char *name, pid_t pid); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args index 3d6c16d..e1c5638 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /tmp/QEMUGuest1.pid -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6e5355a..585eb08 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -156,6 +156,8 @@ mymain(int argc, char **argv) if ((driver.caps = testQemuCapsInit()) == NULL) return EXIT_FAILURE; + if((driver.stateDir = strdup("/tmp")) == NULL) + return EXIT_FAILURE; #define DO_TEST(name, extraFlags) \ do { \ -- 1.6.0.2
On Fri, 2008-12-12 at 19:26 +0100, Guido Günther wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ca96223..a2e573e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -220,6 +220,10 @@ qemudStartup(void) {
if ((base = strdup (SYSCONF_DIR "/libvirt")) == NULL) goto out_of_memory; + + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", LOCAL_STATE_DIR) == -1) + goto out_of_memory;
You need to explicitly set qemu_driver->stateDir to NULL if asprintf fails, since it leaves the pointer in an undefined state.
+ + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", base) == -1) + goto out_of_memory; + }
Same here David
Guido Günther <agx@sigxcpu.org> wrote:
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args index 3d6c16d..e1c5638 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /tmp/QEMUGuest1.pid -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6e5355a..585eb08 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -156,6 +156,8 @@ mymain(int argc, char **argv)
if ((driver.caps = testQemuCapsInit()) == NULL) return EXIT_FAILURE; + if((driver.stateDir = strdup("/tmp")) == NULL) + return EXIT_FAILURE;
#define DO_TEST(name, extraFlags) \ do { \
Hi Guido, Please don't use a world-writable directory like /tmp for this, since someone running this test on a multi-user system would then be vulnerable to a symlink attack for any predictably-named file it creates in that directory.
On Fri, Dec 12, 2008 at 08:42:10PM +0100, Jim Meyering wrote:
Guido Günther <agx@sigxcpu.org> wrote:
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args index 3d6c16d..e1c5638 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /tmp/QEMUGuest1.pid -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6e5355a..585eb08 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -156,6 +156,8 @@ mymain(int argc, char **argv)
if ((driver.caps = testQemuCapsInit()) == NULL) return EXIT_FAILURE; + if((driver.stateDir = strdup("/tmp")) == NULL) + return EXIT_FAILURE;
#define DO_TEST(name, extraFlags) \ do { \
Hi Guido,
Please don't use a world-writable directory like /tmp for this, since someone running this test on a multi-user system would then be vulnerable to a symlink attack for any predictably-named file it creates in that directory.
Agree in general. This case is OK actually ok, and neccessary because we dont actually launch QEMU from the test suite - we merely generate various ARGV sets and compare them to expected output. So we do in fact need a predictable, static path for purposes of the comparison. 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 :|
Guido Günther <agx@sigxcpu.org> wrote:
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args index 3d6c16d..e1c5638 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /tmp/QEMUGuest1.pid -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -usbdevice host:014.006 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 6e5355a..585eb08 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -156,6 +156,8 @@ mymain(int argc, char **argv)
if ((driver.caps = testQemuCapsInit()) == NULL) return EXIT_FAILURE; + if((driver.stateDir = strdup("/tmp")) == NULL) + return EXIT_FAILURE;
#define DO_TEST(name, extraFlags) \ do { \
Hi Guido,
Please don't use a world-writable directory like /tmp for this, since someone running this test on a multi-user system would then be vulnerable to a symlink attack for any predictably-named file it creates in that directory. No file is acutally written there, but I'll change that to "/nowhere" so
On Fri, Dec 12, 2008 at 08:42:10PM +0100, Jim Meyering wrote: there isn't any confiusion about this. -- Guido
On Fri, Dec 12, 2008 at 11:35:03AM -0800, David Lutterkort wrote:
On Fri, 2008-12-12 at 19:26 +0100, Guido Günther wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index ca96223..a2e573e 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -220,6 +220,10 @@ qemudStartup(void) {
if ((base = strdup (SYSCONF_DIR "/libvirt")) == NULL) goto out_of_memory; + + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", LOCAL_STATE_DIR) == -1) + goto out_of_memory;
You need to explicitly set qemu_driver->stateDir to NULL if asprintf fails, since it leaves the pointer in an undefined state.
+ + if (asprintf (&qemu_driver->stateDir, + "%s/run/libvirt/qemu/", base) == -1) + goto out_of_memory; + }
Same here Done via virAsprintf. Thanks for pointing this out. -- Guido
On Fri, Dec 12, 2008 at 07:26:06PM +0100, Guido G?nther wrote:
@@ -870,6 +874,9 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT("-monitor"); ADD_ARG_LIT("pty");
+ ADD_ARG_LIT("-pidfile"); + ADD_ARG(pidfile); +
Not all versions of QEMU support the -pidfile argument. We'll have to add another probe to qemudExtractVersionInfo() method as we do for -drive, -uuid, etc. I'm fine saying that older QEMU instances won't survive a daemon restart, so we can just leave off -pidfile for those that don't support it. 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 :|
On Mon, Dec 15, 2008 at 11:15:53AM +0000, Daniel P. Berrange wrote:
On Fri, Dec 12, 2008 at 07:26:06PM +0100, Guido G?nther wrote:
@@ -870,6 +874,9 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT("-monitor"); ADD_ARG_LIT("pty");
+ ADD_ARG_LIT("-pidfile"); + ADD_ARG(pidfile); +
Not all versions of QEMU support the -pidfile argument. We'll have to add another probe to qemudExtractVersionInfo() method as we do for -drive, -uuid, etc. I skipped this since -pidfile got introduced 2004 already with:
svn://svn.savannah.nongnu.org/qemu/trunk@1166 Do we really support that old qemu? -- Guido
On Mon, Dec 15, 2008 at 07:40:05PM +0100, Guido G?nther wrote:
On Mon, Dec 15, 2008 at 11:15:53AM +0000, Daniel P. Berrange wrote:
On Fri, Dec 12, 2008 at 07:26:06PM +0100, Guido G?nther wrote:
@@ -870,6 +874,9 @@ int qemudBuildCommandLine(virConnectPtr conn, ADD_ARG_LIT("-monitor"); ADD_ARG_LIT("pty");
+ ADD_ARG_LIT("-pidfile"); + ADD_ARG(pidfile); +
Not all versions of QEMU support the -pidfile argument. We'll have to add another probe to qemudExtractVersionInfo() method as we do for -drive, -uuid, etc. I skipped this since -pidfile got introduced 2004 already with:
Opps, my mistake - it was -daemon that was added in 0.9.0, not -pidfile
svn://svn.savannah.nongnu.org/qemu/trunk@1166
Do we really support that old qemu?
No, 0.8.2 is the first version I intended it to work with, since that's what was in Fedora 6 / EPEL-5 when I wrote QEMU driver. 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 :|
On Mon, Dec 15, 2008 at 07:09:10PM +0000, Daniel P. Berrange wrote: [..snip..]
I skipped this since -pidfile got introduced 2004 already with:
Opps, my mistake - it was -daemon that was added in 0.9.0, not -pidfile
svn://svn.savannah.nongnu.org/qemu/trunk@1166
Do we really support that old qemu?
No, 0.8.2 is the first version I intended it to work with, since that's what was in Fedora 6 / EPEL-5 when I wrote QEMU driver. O.k. to apply the attaced version then? Changes are: * use virAsprintf * dont use /tmp but /nowhere * fix all testcases Cheers, -- Guido
On Thu, Dec 18, 2008 at 10:31:35AM +0100, Guido G?nther wrote: > On Mon, Dec 15, 2008 at 07:09:10PM +0000, Daniel P. Berrange wrote: > [..snip..] > > > I skipped this since -pidfile got introduced 2004 already with: > > > > Opps, my mistake - it was -daemon that was added in 0.9.0, not -pidfile > > > > > svn://svn.savannah.nongnu.org/qemu/trunk@1166 > > > > > > Do we really support that old qemu? > > > > No, 0.8.2 is the first version I intended it to work with, since that's > > what was in Fedora 6 / EPEL-5 when I wrote QEMU driver. > O.k. to apply the attaced version then? Changes are: > * use virAsprintf > * dont use /tmp but /nowhere > * fix all testcases Yeah, fine by me 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 :|
On Thu, Dec 18, 2008 at 10:24:57AM +0000, Daniel P. Berrange wrote: > On Thu, Dec 18, 2008 at 10:31:35AM +0100, Guido G?nther wrote: > > On Mon, Dec 15, 2008 at 07:09:10PM +0000, Daniel P. Berrange wrote: > > [..snip..] > > > > I skipped this since -pidfile got introduced 2004 already with: > > > > > > Opps, my mistake - it was -daemon that was added in 0.9.0, not -pidfile > > > > > > > svn://svn.savannah.nongnu.org/qemu/trunk@1166 > > > > > > > > Do we really support that old qemu? > > > > > > No, 0.8.2 is the first version I intended it to work with, since that's > > > what was in Fedora 6 / EPEL-5 when I wrote QEMU driver. > > O.k. to apply the attaced version then? Changes are: > > * use virAsprintf > > * dont use /tmp but /nowhere > > * fix all testcases > > Yeah, fine by me Applied now. -- Guido
participants (4)
-
Daniel P. Berrange -
David Lutterkort -
Guido Günther -
Jim Meyering