[libvirt] [PATCH] qemuProcessReconnect: Fill in pid file path

2015-03-03 Thread Michal Privoznik 
https://bugzilla.redhat.com/show_bug.cgi?id=1197600

So, libvirt uses pid file to track pid of started qemus. Whenever
a domain is started, its pid is put into corresponding pid file.
The pid file path is generated based on domain name and stored
into domain object internals. However, it's not stored in the
status XML and therefore lost on daemon restarts. Hence, later,
when domain is being shut down, the daemon does not know which
pid file to unlink, and the correct pid file is left behind. To
avoid this, lets generate the pid file path again in
qemuProcessReconnect().

Reported-by: Luyao Huang lhu...@redhat.com
Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
 src/qemu/qemu_process.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7ce6115..e77616a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3762,6 +3762,13 @@ qemuProcessReconnect(void *opaque)
 cfg = virQEMUDriverGetConfig(driver);
 priv = obj-privateData;
 
+/* XXX If we ever gonna change pid file pattern, come up with
+ * some intelligence here to deal with old paths. */
+if (!(priv-pidfile = virPidFileBuildPath(cfg-stateDir, obj-def-name))) 
{
+virReportSystemError(errno, %s, _(Failed to build pidfile path.));
+goto killvm;
+}
+
 if (qemuDomainObjBeginJob(driver, obj, QEMU_JOB_MODIFY)  0)
 goto killvm;
 
-- 
2.0.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuProcessReconnect: Fill in pid file path

2015-03-03 Thread Jiri Denemark 
On Tue, Mar 03, 2015 at 11:56:37 +0100, Michal Privoznik wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=1197600
 
 So, libvirt uses pid file to track pid of started qemus. Whenever
 a domain is started, its pid is put into corresponding pid file.
 The pid file path is generated based on domain name and stored
 into domain object internals. However, it's not stored in the
 status XML and therefore lost on daemon restarts. Hence, later,
 when domain is being shut down, the daemon does not know which
 pid file to unlink, and the correct pid file is left behind. To
 avoid this, lets generate the pid file path again in
 qemuProcessReconnect().

Right, this is what I wanted to suggest after seeing the original patch.

 
 Reported-by: Luyao Huang lhu...@redhat.com
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
  src/qemu/qemu_process.c | 7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 7ce6115..e77616a 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -3762,6 +3762,13 @@ qemuProcessReconnect(void *opaque)
  cfg = virQEMUDriverGetConfig(driver);
  priv = obj-privateData;
  
 +/* XXX If we ever gonna change pid file pattern, come up with
 + * some intelligence here to deal with old paths. */
 +if (!(priv-pidfile = virPidFileBuildPath(cfg-stateDir, 
 obj-def-name))) {
 +virReportSystemError(errno, %s, _(Failed to build pidfile 
 path.));

Just remove this line. virPidFileBuildPath already reports the error.

 +goto killvm;
 +}
 +
  if (qemuDomainObjBeginJob(driver, obj, QEMU_JOB_MODIFY)  0)
  goto killvm;

ACK

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuProcessReconnect: Fill in pid file path

2015-03-03 Thread Daniel P. Berrange 
On Tue, Mar 03, 2015 at 11:56:37AM +0100, Michal Privoznik wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=1197600
 
 So, libvirt uses pid file to track pid of started qemus. Whenever
 a domain is started, its pid is put into corresponding pid file.
 The pid file path is generated based on domain name and stored
 into domain object internals. However, it's not stored in the
 status XML and therefore lost on daemon restarts. Hence, later,
 when domain is being shut down, the daemon does not know which
 pid file to unlink, and the correct pid file is left behind. To
 avoid this, lets generate the pid file path again in
 qemuProcessReconnect().
 
 Reported-by: Luyao Huang lhu...@redhat.com
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
  src/qemu/qemu_process.c | 7 +++
  1 file changed, 7 insertions(+)
 
 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 7ce6115..e77616a 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -3762,6 +3762,13 @@ qemuProcessReconnect(void *opaque)
  cfg = virQEMUDriverGetConfig(driver);
  priv = obj-privateData;
  
 +/* XXX If we ever gonna change pid file pattern, come up with
 + * some intelligence here to deal with old paths. */

We already have arbitrary patterns due to virDomainQEMUAttach() which can
talk to an existing QEMU.  We should save the pidfile pattern in the status
XML file so we remember it. For upgrades from existing deployments we would
have to cope with the status XML not having pidfile, and so fallback to
building it again.

 +if (!(priv-pidfile = virPidFileBuildPath(cfg-stateDir, 
 obj-def-name))) {
 +virReportSystemError(errno, %s, _(Failed to build pidfile 
 path.));
 +goto killvm;
 +}
 +
  if (qemuDomainObjBeginJob(driver, obj, QEMU_JOB_MODIFY)  0)
  goto killvm;


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list