[Qemu-devel] [PATCH 0/2] Couple of qemu-pr-helper fixes

[Qemu-devel] [PATCH 0/2] Couple of qemu-pr-helper fixes

[Michal Privoznik]

The first one is trivial.

The second is slightly trickier. Libvirt has this virCommand subsystem
(found under src/util/vircommand.c) which it uses to spawn commands. The
subsystem allows libvirt to both daemonize a process and learn its PID.
The latter of course does not work if command daemonizes itself (hence
libvirt will not use qemu-pr-helper --daemon). BUT, we still need
qemu-pr-helper to lock the pidfile (so that when libvirt tries to lock
it it is denied). NB, libvirt uses fnctl(cmd = F_SETLK) to lock
pidfiles. So leaking locked FD into qemu-pr-helper is not an option
because that doesn't survive fork().

Another, trivial reason might be that if I run:

  qemu-pr-helper --pidfile /tmp/pr.pid

(without --daemon), I'd still expect qemu-pr-helper to write pidfile
because I told it to.

Michal Privoznik (2):
  qemu-pr-helper: Daemonize before dropping privileges
  qemu-pr-helper: Write pidfile more often

 scsi/qemu-pr-helper.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

-- 
2.16.1

[Qemu-devel] [PATCH 1/2] qemu-pr-helper: Daemonize before dropping privileges

[Michal Privoznik]

After we've dropped privileges it might be not possible to write
pidfile. For instance, if this binary is run as root (because
user wants it to write pidfile to some privileged location)
writing pidfile fails because privileges are dropped before we
even get to that.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 scsi/qemu-pr-helper.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 21e1b8ea60..eeff80acf2 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -1081,13 +1081,6 @@ int main(int argc, char **argv)
                                          accept_client,
                                          NULL, NULL);
 
-#ifdef CONFIG_LIBCAP
-    if (drop_privileges() < 0) {
-        error_report("Failed to drop privileges: %s", strerror(errno));
-        exit(EXIT_FAILURE);
-    }
-#endif
-
     if (daemonize) {
         if (daemon(0, 0) < 0) {
             error_report("Failed to daemonize: %s", strerror(errno));
@@ -1096,6 +1089,13 @@ int main(int argc, char **argv)
         write_pidfile();
     }
 
+#ifdef CONFIG_LIBCAP
+    if (drop_privileges() < 0) {
+        error_report("Failed to drop privileges: %s", strerror(errno));
+        exit(EXIT_FAILURE);
+    }
+#endif
+
     state = RUNNING;
     do {
         main_loop_wait(false);
-- 
2.16.1

[Qemu-devel] [PATCH 2/2] qemu-pr-helper: Write pidfile more often

[Michal Privoznik]

Let's write pidfile even if user did not request --daemon but
they requested just --pidfile. Libvirt will use exactly this.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 scsi/qemu-pr-helper.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index eeff80acf2..d0f83176e1 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -924,6 +924,7 @@ int main(int argc, char **argv)
     Error *local_err = NULL;
     char *trace_file = NULL;
     bool daemonize = false;
+    bool pidfile_specified = false;
     unsigned socket_activation;
 
     struct sigaction sa_sigterm;
@@ -954,6 +955,7 @@ int main(int argc, char **argv)
         case 'f':
             g_free(pidfile);
             pidfile = g_strdup(optarg);
+            pidfile_specified = true;
             break;
 #ifdef CONFIG_LIBCAP
         case 'u': {
@@ -1086,9 +1088,11 @@ int main(int argc, char **argv)
             error_report("Failed to daemonize: %s", strerror(errno));
             exit(EXIT_FAILURE);
         }
-        write_pidfile();
     }
 
+    if (daemonize || pidfile_specified)
+        write_pidfile();
+
 #ifdef CONFIG_LIBCAP
     if (drop_privileges() < 0) {
         error_report("Failed to drop privileges: %s", strerror(errno));
-- 
2.16.1

Re: [Qemu-devel] [PATCH 0/2] Couple of qemu-pr-helper fixes

[Fam Zheng]

On Tue, 04/03 15:12, Michal Privoznik wrote:
> The first one is trivial.
> 
> The second is slightly trickier. Libvirt has this virCommand subsystem
> (found under src/util/vircommand.c) which it uses to spawn commands. The
> subsystem allows libvirt to both daemonize a process and learn its PID.
> The latter of course does not work if command daemonizes itself (hence
> libvirt will not use qemu-pr-helper --daemon). BUT, we still need
> qemu-pr-helper to lock the pidfile (so that when libvirt tries to lock
> it it is denied). NB, libvirt uses fnctl(cmd = F_SETLK) to lock
> pidfiles. So leaking locked FD into qemu-pr-helper is not an option
> because that doesn't survive fork().
> 
> Another, trivial reason might be that if I run:
> 
>   qemu-pr-helper --pidfile /tmp/pr.pid
> 
> (without --daemon), I'd still expect qemu-pr-helper to write pidfile
> because I told it to.
> 
> Michal Privoznik (2):
>   qemu-pr-helper: Daemonize before dropping privileges
>   qemu-pr-helper: Write pidfile more often

Reviewed-by: Fam Zheng <famz@redhat.com>