[PATCH v2 0/5] qemu_passt: Fix issues with PID file

[PATCH v2 0/5] qemu_passt: Fix issues with PID file

[Michal Privoznik]

This is a v2 of:

https://listman.redhat.com/archives/libvir-list/2023-February/237731.html

diff to v1:
- Merged patches that were ACKed in v1,
- Dropped 4/4 from the original series (the one that sets --foreground),
  and implemented a different approach

Michal Prívozník (5):
  qemu_passt: Avoid double daemonizing passt
  qemu_passt: Report passt's error on failed start
  qemu_passt: Make passt report errors to stderr whenever possible
  qemu_passt: Deduplicate passt killing code
  qemu_passt: Let passt write the PID file

 src/qemu/qemu_passt.c | 61 +++++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 20 deletions(-)

-- 
2.39.1

[PATCH v2 1/5] qemu_passt: Avoid double daemonizing passt

[Michal Privoznik]

When passt is started, it daemonizes itself by default. There's
no point in having our virCommand module daemonize it too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_passt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index 78830fdc26..adc69fc052 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -156,7 +156,6 @@ qemuPasstStart(virDomainObj *vm,
     virCommandClearCaps(cmd);
     virCommandSetPidFile(cmd, pidfile);
     virCommandSetErrorFD(cmd, &errfd);
-    virCommandDaemonize(cmd);
 
     virCommandAddArgList(cmd,
                          "--one-off",
-- 
@@ -156,7 +156,6 @@ qemuPasstStart(virDomainObj *vm,
     virCommandClearCaps(cmd);
     virCommandSetPidFile(cmd, pidfile);
     virCommandSetErrorFD(cmd, &errfd);
-    virCommandDaemonize(cmd);
 
     virCommandAddArgList(cmd,
                          "--one-off",
-- 
2.39.1

[PATCH v2 2/5] qemu_passt: Report passt's error on failed start

[Michal Privoznik]

When starting passt, it may write something onto its stderr
(convincing it to print even more is addressed later). Pass this
string we read to user.

Since we're not daemonizing passt anymore (see previous commit),
we can let virCommand module do all the heavy lifting and switch
to virCommandSetErrorBuffer() instead of reading error from an
FD.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_passt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index adc69fc052..c082c149cd 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -144,18 +144,18 @@ qemuPasstStart(virDomainObj *vm,
     g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
     g_autoptr(virCommand) cmd = NULL;
     g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
+    g_autofree char *errbuf = NULL;
     char macaddr[VIR_MAC_STRING_BUFLEN];
     size_t i;
     pid_t pid = (pid_t) -1;
     int exitstatus = 0;
     int cmdret = 0;
-    VIR_AUTOCLOSE errfd = -1;
 
     cmd = virCommandNew(PASST);
 
     virCommandClearCaps(cmd);
     virCommandSetPidFile(cmd, pidfile);
-    virCommandSetErrorFD(cmd, &errfd);
+    virCommandSetErrorBuffer(cmd, &errbuf);
 
     virCommandAddArgList(cmd,
                          "--one-off",
@@ -266,7 +266,7 @@ qemuPasstStart(virDomainObj *vm,
 
     if (cmdret < 0 || exitstatus != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Could not start 'passt'. exitstatus: %d"), exitstatus);
+                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
         goto error;
     }
 
-- 
@@ -144,18 +144,18 @@ qemuPasstStart(virDomainObj *vm,
     g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
     g_autoptr(virCommand) cmd = NULL;
     g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
+    g_autofree char *errbuf = NULL;
     char macaddr[VIR_MAC_STRING_BUFLEN];
     size_t i;
     pid_t pid = (pid_t) -1;
     int exitstatus = 0;
     int cmdret = 0;
-    VIR_AUTOCLOSE errfd = -1;
 
     cmd = virCommandNew(PASST);
 
     virCommandClearCaps(cmd);
     virCommandSetPidFile(cmd, pidfile);
-    virCommandSetErrorFD(cmd, &errfd);
+    virCommandSetErrorBuffer(cmd, &errbuf);
 
     virCommandAddArgList(cmd,
                          "--one-off",
@@ -266,7 +266,7 @@ qemuPasstStart(virDomainObj *vm,
 
     if (cmdret < 0 || exitstatus != 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Could not start 'passt'. exitstatus: %d"), exitstatus);
+                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
         goto error;
     }
 
-- 
2.39.1

[PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Michal Privoznik]

Passt has '--stderr' argument which makes it report error onto
stderr rather to system log. Unfortunately, it's currently
impossible to use both '--log-file' and '--stderr', so pass the
latter only if the former isn't passed. Then, use the stderr to
produce more user friendly error message on failed start.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index c082c149cd..881205449b 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
     if (net->sourceDev)
         virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
 
-    if (net->backend.logFile)
+    if (net->backend.logFile) {
         virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
+    } else {
+        /* By default, passt logs into system logger. But we are interested
+         * into errors too. Make it print errors onto stderr. */
+        virCommandAddArg(cmd, "--stderr");
+    }
 
     /* Add IP address info */
     for (i = 0; i < net->guestIP.nips; i++) {
@@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
         goto error;
 
     if (cmdret < 0 || exitstatus != 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
+        if (STRNEQ_NULLABLE(errbuf, "")) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Could not start 'passt': %s"), errbuf);
+        } else if (net->backend.logFile) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Could not start 'passt': look into %s for error"),
+                           net->backend.logFile);
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Could not start 'passt': exit status = '%d'"),
+                           exitstatus);
+        }
+
         goto error;
     }
 
-- 
@@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
     if (net->sourceDev)
         virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
 
-    if (net->backend.logFile)
+    if (net->backend.logFile) {
         virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
+    } else {
+        /* By default, passt logs into system logger. But we are interested
+         * into errors too. Make it print errors onto stderr. */
+        virCommandAddArg(cmd, "--stderr");
+    }
 
     /* Add IP address info */
     for (i = 0; i < net->guestIP.nips; i++) {
@@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
         goto error;
 
     if (cmdret < 0 || exitstatus != 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
+        if (STRNEQ_NULLABLE(errbuf, "")) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Could not start 'passt': %s"), errbuf);
+        } else if (net->backend.logFile) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Could not start 'passt': look into %s for error"),
+                           net->backend.logFile);
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Could not start 'passt': exit status = '%d'"),
+                           exitstatus);
+        }
+
         goto error;
     }
 
-- 
2.39.1

[PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

[Michal Privoznik]

There are two places where we kill passt:

1) qemuPasstStop() - called transitively from qemuProcessStop(),
2) qemuPasstStart() - after failed start.

Now, the code from 2) lack error preservation (so if there's
another error during cleanup we might overwrite the original
error). Therefore, move the internals of qemuPasstStop() into a
separate function and call it from both places.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_passt.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index 881205449b..a4cc9e7166 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
 }
 
 
-void
-qemuPasstStop(virDomainObj *vm,
-              virDomainNetDef *net)
+static void
+qemuPasstKill(const char *pidfile)
 {
-    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
     virErrorPtr orig_err;
 
     virErrorPreserveLast(&orig_err);
@@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
 }
 
 
+void
+qemuPasstStop(virDomainObj *vm,
+              virDomainNetDef *net)
+{
+    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
+
+    qemuPasstKill(pidfile);
+}
+
+
 int
 qemuPasstSetupCgroup(virDomainObj *vm,
                      virDomainNetDef *net,
@@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
     g_autofree char *errbuf = NULL;
     char macaddr[VIR_MAC_STRING_BUFLEN];
     size_t i;
-    pid_t pid = (pid_t) -1;
     int exitstatus = 0;
     int cmdret = 0;
 
@@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
     return 0;
 
  error:
-    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
-    if (pid != -1)
-        virProcessKillPainfully(pid, true);
-    unlink(pidfile);
-
+    qemuPasstKill(pidfile);
     return -1;
 }
-- 
@@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
 }
 
 
-void
-qemuPasstStop(virDomainObj *vm,
-              virDomainNetDef *net)
+static void
+qemuPasstKill(const char *pidfile)
 {
-    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
     virErrorPtr orig_err;
 
     virErrorPreserveLast(&orig_err);
@@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
 }
 
 
+void
+qemuPasstStop(virDomainObj *vm,
+              virDomainNetDef *net)
+{
+    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
+
+    qemuPasstKill(pidfile);
+}
+
+
 int
 qemuPasstSetupCgroup(virDomainObj *vm,
                      virDomainNetDef *net,
@@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
     g_autofree char *errbuf = NULL;
     char macaddr[VIR_MAC_STRING_BUFLEN];
     size_t i;
-    pid_t pid = (pid_t) -1;
     int exitstatus = 0;
     int cmdret = 0;
 
@@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
     return 0;
 
  error:
-    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
-    if (pid != -1)
-        virProcessKillPainfully(pid, true);
-    unlink(pidfile);
-
+    qemuPasstKill(pidfile);
     return -1;
 }
-- 
2.39.1

[PATCH v2 5/5] qemu_passt: Let passt write the PID file

[Michal Privoznik]

The way we start passt currently is: we use
virCommandSetPidFile() to use our virCommand machinery to acquire
the PID file and leak opened FD into passt. Then, we use
virPidFile*() APIs to read the PID file (which is needed when
placing it into CGroups or killing it). But this does not fly
really because passt daemonizes itself. Thus the process we
started dies soon and thus the PID file is closed and unlocked.

We could work around this by passing '--foreground' argument, but
that weakens passt as it can't create new PID namespace (because
it doesn't fork()).

The solution is to let passt write the PID file, but since it
does not lock the file and closes it as soon as it is written, we
have to switch to those virPidFile APIs which don't expect PID
file to be locked.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_passt.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index a4cc9e7166..47f4b5fcae 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -72,7 +72,7 @@ qemuPasstGetPid(virDomainObj *vm,
 {
     g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
 
-    return virPidFileReadPathIfLocked(pidfile, pid);
+    return virPidFileReadPath(pidfile, pid);
 }
 
 
@@ -106,11 +106,14 @@ static void
 qemuPasstKill(const char *pidfile)
 {
     virErrorPtr orig_err;
+    pid_t pid = 0;
 
     virErrorPreserveLast(&orig_err);
 
-    if (virPidFileForceCleanupPath(pidfile) < 0)
-        VIR_WARN("Unable to kill passt process");
+    ignore_value(virPidFileReadPath(pidfile, &pid));
+    if (pid != 0)
+        virProcessKillPainfully(pid, true);
+    unlink(pidfile);
 
     virErrorRestore(&orig_err);
 }
@@ -161,13 +164,13 @@ qemuPasstStart(virDomainObj *vm,
     cmd = virCommandNew(PASST);
 
     virCommandClearCaps(cmd);
-    virCommandSetPidFile(cmd, pidfile);
     virCommandSetErrorBuffer(cmd, &errbuf);
 
     virCommandAddArgList(cmd,
                          "--one-off",
                          "--socket", passtSocketName,
                          "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
+                         "--pid", pidfile,
                          NULL);
 
     if (net->mtu) {
-- 
@@ -72,7 +72,7 @@ qemuPasstGetPid(virDomainObj *vm,
 {
     g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
 
-    return virPidFileReadPathIfLocked(pidfile, pid);
+    return virPidFileReadPath(pidfile, pid);
 }
 
 
@@ -106,11 +106,14 @@ static void
 qemuPasstKill(const char *pidfile)
 {
     virErrorPtr orig_err;
+    pid_t pid = 0;
 
     virErrorPreserveLast(&orig_err);
 
-    if (virPidFileForceCleanupPath(pidfile) < 0)
-        VIR_WARN("Unable to kill passt process");
+    ignore_value(virPidFileReadPath(pidfile, &pid));
+    if (pid != 0)
+        virProcessKillPainfully(pid, true);
+    unlink(pidfile);
 
     virErrorRestore(&orig_err);
 }
@@ -161,13 +164,13 @@ qemuPasstStart(virDomainObj *vm,
     cmd = virCommandNew(PASST);
 
     virCommandClearCaps(cmd);
-    virCommandSetPidFile(cmd, pidfile);
     virCommandSetErrorBuffer(cmd, &errbuf);
 
     virCommandAddArgList(cmd,
                          "--one-off",
                          "--socket", passtSocketName,
                          "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
+                         "--pid", pidfile,
                          NULL);
 
     if (net->mtu) {
-- 
2.39.1

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Jonathon Jongsma]

On 2/16/23 7:32 AM, Michal Privoznik wrote:
> Passt has '--stderr' argument which makes it report error onto
> stderr rather to system log. Unfortunately, it's currently
> impossible to use both '--log-file' and '--stderr', so pass the
> latter only if the former isn't passed. Then, use the stderr to
> produce more user friendly error message on failed start.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index c082c149cd..881205449b 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
>       if (net->sourceDev)
>           virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
>   
> -    if (net->backend.logFile)
> +    if (net->backend.logFile) {
>           virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
> +    } else {
> +        /* By default, passt logs into system logger. But we are interested
> +         * into errors too. Make it print errors onto stderr. */

s/into/in the/ ?

> +        virCommandAddArg(cmd, "--stderr");
> +    }
>   
>       /* Add IP address info */
>       for (i = 0; i < net->guestIP.nips; i++) {
> @@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
>           goto error;
>   
>       if (cmdret < 0 || exitstatus != 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
> +        if (STRNEQ_NULLABLE(errbuf, "")) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': %s"), errbuf);
> +        } else if (net->backend.logFile) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': look into %s for error"),
> +                           net->backend.logFile);
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': exit status = '%d'"),
> +                           exitstatus);
> +        }
> +
>           goto error;
>       }
>   

Re: [PATCH v2 1/5] qemu_passt: Avoid double daemonizing passt

[Stefano Brivio]

On Thu, 16 Feb 2023 14:32:48 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> When passt is started, it daemonizes itself by default. There's
> no point in having our virCommand module daemonize it too.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_passt.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 78830fdc26..adc69fc052 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -156,7 +156,6 @@ qemuPasstStart(virDomainObj *vm,
>      virCommandClearCaps(cmd);
>      virCommandSetPidFile(cmd, pidfile);
>      virCommandSetErrorFD(cmd, &errfd);
> -    virCommandDaemonize(cmd);
>  
>      virCommandAddArgList(cmd,
>                           "--one-off",

For what it's worth,

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [PATCH v2 2/5] qemu_passt: Report passt's error on failed start

[Stefano Brivio]

On Thu, 16 Feb 2023 14:32:49 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> When starting passt, it may write something onto its stderr
> (convincing it to print even more is addressed later). Pass this
> string we read to user.
> 
> Since we're not daemonizing passt anymore (see previous commit),
> we can let virCommand module do all the heavy lifting and switch
> to virCommandSetErrorBuffer() instead of reading error from an
> FD.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Stefano Brivio]

On Thu, 16 Feb 2023 14:32:50 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> Passt has '--stderr' argument which makes it report error onto
> stderr rather to system log. Unfortunately, it's currently
> impossible to use both '--log-file' and '--stderr', so pass the
> latter only if the former isn't passed. Then, use the stderr to
> produce more user friendly error message on failed start.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index c082c149cd..881205449b 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
>      if (net->sourceDev)
>          virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
>  
> -    if (net->backend.logFile)
> +    if (net->backend.logFile) {
>          virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
> +    } else {
> +        /* By default, passt logs into system logger. But we are interested
> +         * into errors too. Make it print errors onto stderr. */
> +        virCommandAddArg(cmd, "--stderr");
> +    }

There's no need for this, see my previous email, archived at:
  https://listman.redhat.com/archives/libvir-list/2023-February/237880.html

>  
>      /* Add IP address info */
>      for (i = 0; i < net->guestIP.nips; i++) {
> @@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
>          goto error;
>  
>      if (cmdret < 0 || exitstatus != 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
> +        if (STRNEQ_NULLABLE(errbuf, "")) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': %s"), errbuf);
> +        } else if (net->backend.logFile) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': look into %s for error"),
> +                           net->backend.logFile);

...and this won't be needed either, with Laine's series for passt. It
might actually be a bit misleading.

> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': exit status = '%d'"),
> +                           exitstatus);
> +        }
> +
>          goto error;
>      }
>  

So all in all I think this is unnecessary, but also kind of harmless.

-- 
Stefano

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

[Stefano Brivio]

On Thu, 16 Feb 2023 14:32:51 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> There are two places where we kill passt:
> 
> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
> 2) qemuPasstStart() - after failed start.
> 
> Now, the code from 2) lack error preservation (so if there's
> another error during cleanup we might overwrite the original
> error). Therefore, move the internals of qemuPasstStop() into a
> separate function and call it from both places.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_passt.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 881205449b..a4cc9e7166 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
>  }
>  
>  
> -void
> -qemuPasstStop(virDomainObj *vm,
> -              virDomainNetDef *net)
> +static void
> +qemuPasstKill(const char *pidfile)

A minor comment, should you respin: I think it should be made clear that
this is not the expected/normal way in which passt will terminate --
here or in the next patch. Removing the PID file is nice, but that's
(usually) about it.

>  {
> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>      virErrorPtr orig_err;
>  
>      virErrorPreserveLast(&orig_err);
> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
>  }
>  
>  
> +void
> +qemuPasstStop(virDomainObj *vm,
> +              virDomainNetDef *net)
> +{
> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> +
> +    qemuPasstKill(pidfile);
> +}
> +
> +
>  int
>  qemuPasstSetupCgroup(virDomainObj *vm,
>                       virDomainNetDef *net,
> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
>      g_autofree char *errbuf = NULL;
>      char macaddr[VIR_MAC_STRING_BUFLEN];
>      size_t i;
> -    pid_t pid = (pid_t) -1;
>      int exitstatus = 0;
>      int cmdret = 0;
>  
> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
>      return 0;
>  
>   error:
> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
> -    if (pid != -1)
> -        virProcessKillPainfully(pid, true);
> -    unlink(pidfile);
> -
> +    qemuPasstKill(pidfile);

...what takes care of terminating passt in case qemu doesn't start, now?
The fact that the process is in the cgroup, right?

>      return -1;
>  }

-- 
Stefano

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Michal Prívozník]

On 2/16/23 16:42, Jonathon Jongsma wrote:
> On 2/16/23 7:32 AM, Michal Privoznik wrote:
>> Passt has '--stderr' argument which makes it report error onto
>> stderr rather to system log. Unfortunately, it's currently
>> impossible to use both '--log-file' and '--stderr', so pass the
>> latter only if the former isn't passed. Then, use the stderr to
>> produce more user friendly error message on failed start.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
>> index c082c149cd..881205449b 100644
>> --- a/src/qemu/qemu_passt.c
>> +++ b/src/qemu/qemu_passt.c
>> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
>>       if (net->sourceDev)
>>           virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
>>   -    if (net->backend.logFile)
>> +    if (net->backend.logFile) {
>>           virCommandAddArgList(cmd, "--log-file",
>> net->backend.logFile, NULL);
>> +    } else {
>> +        /* By default, passt logs into system logger. But we are
>> interested
>> +         * into errors too. Make it print errors onto stderr. */
> 
> s/into/in the/ ?

Honestly, I have no idea. I'm not a native speaker. Maybe it's 'print
onto paper' but 'print into a stream'?

Anyway, fixed locally. Thanks.

Michal

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Michal Prívozník]

On 2/16/23 17:07, Stefano Brivio wrote:
> On Thu, 16 Feb 2023 14:32:50 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> Passt has '--stderr' argument which makes it report error onto
>> stderr rather to system log. Unfortunately, it's currently
>> impossible to use both '--log-file' and '--stderr', so pass the
>> latter only if the former isn't passed. Then, use the stderr to
>> produce more user friendly error message on failed start.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
>> index c082c149cd..881205449b 100644
>> --- a/src/qemu/qemu_passt.c
>> +++ b/src/qemu/qemu_passt.c
>> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
>>      if (net->sourceDev)
>>          virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
>>  
>> -    if (net->backend.logFile)
>> +    if (net->backend.logFile) {
>>          virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
>> +    } else {
>> +        /* By default, passt logs into system logger. But we are interested
>> +         * into errors too. Make it print errors onto stderr. */
>> +        virCommandAddArg(cmd, "--stderr");
>> +    }
> 
> There's no need for this, see my previous email, archived at:
>   https://listman.redhat.com/archives/libvir-list/2023-February/237880.html
> 
>>  
>>      /* Add IP address info */
>>      for (i = 0; i < net->guestIP.nips; i++) {
>> @@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
>>          goto error;
>>  
>>      if (cmdret < 0 || exitstatus != 0) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
>> +        if (STRNEQ_NULLABLE(errbuf, "")) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Could not start 'passt': %s"), errbuf);
>> +        } else if (net->backend.logFile) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Could not start 'passt': look into %s for error"),
>> +                           net->backend.logFile);
> 
> ...and this won't be needed either, with Laine's series for passt. It
> might actually be a bit misleading.
> 
>> +        } else {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Could not start 'passt': exit status = '%d'"),
>> +                           exitstatus);
>> +        }
>> +
>>          goto error;
>>      }
>>  
> 
> So all in all I think this is unnecessary, but also kind of harmless.
> 

Except those patches are not merged yet. And as we are getting close to
the release I'd like to make this work with what we have now. We've been
burned plenty of times with QEMU to learn our lesson. We've merged
patches that were based not on QEMU's git, but some patches on top
thinking - yeah, the API won't change. And then it did.

Now we don't require a release (which would be ideal), but at least for
patches to be merged. When they get merged then yeah, this can be reworked.

Michal

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Laine Stump]

This is all unnecessary. The issue with error messages has been fixed 
upstream in passt (along with other logging issues), and just serves to 
unnecessarily complicate the code.

On 2/16/23 8:32 AM, Michal Privoznik wrote:
> Passt has '--stderr' argument which makes it report error onto
> stderr rather to system log. Unfortunately, it's currently
> impossible to use both '--log-file' and '--stderr', so pass the
> latter only if the former isn't passed. Then, use the stderr to
> produce more user friendly error message on failed start.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index c082c149cd..881205449b 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
>       if (net->sourceDev)
>           virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
>   
> -    if (net->backend.logFile)
> +    if (net->backend.logFile) {
>           virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
> +    } else {
> +        /* By default, passt logs into system logger. But we are interested
> +         * into errors too. Make it print errors onto stderr. */
> +        virCommandAddArg(cmd, "--stderr");
> +    }
>   
>       /* Add IP address info */
>       for (i = 0; i < net->guestIP.nips; i++) {
> @@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
>           goto error;
>   
>       if (cmdret < 0 || exitstatus != 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
> +        if (STRNEQ_NULLABLE(errbuf, "")) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': %s"), errbuf);
> +        } else if (net->backend.logFile) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': look into %s for error"),
> +                           net->backend.logFile);
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Could not start 'passt': exit status = '%d'"),
> +                           exitstatus);
> +        }
> +
>           goto error;
>       }
>   

Re: [PATCH v2 1/5] qemu_passt: Avoid double daemonizing passt

[Laine Stump]

On 2/16/23 8:32 AM, Michal Privoznik wrote:
> When passt is started, it daemonizes itself by default. There's
> no point in having our virCommand module daemonize it too.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

> ---
>   src/qemu/qemu_passt.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 78830fdc26..adc69fc052 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -156,7 +156,6 @@ qemuPasstStart(virDomainObj *vm,
>       virCommandClearCaps(cmd);
>       virCommandSetPidFile(cmd, pidfile);
>       virCommandSetErrorFD(cmd, &errfd);
> -    virCommandDaemonize(cmd);
>   
>       virCommandAddArgList(cmd,
>                            "--one-off",

Re: [PATCH v2 0/5] qemu_passt: Fix issues with PID file

[Laine Stump]

On 2/16/23 8:32 AM, Michal Privoznik wrote:
> This is a v2 of:
> 
> https://listman.redhat.com/archives/libvir-list/2023-February/237731.html
> 
> diff to v1:
> - Merged patches that were ACKed in v1,
> - Dropped 4/4 from the original series (the one that sets --foreground),
>    and implemented a different approach
> 
> Michal Prívozník (5):
>    qemu_passt: Avoid double daemonizing passt
>    qemu_passt: Report passt's error on failed start
>    qemu_passt: Make passt report errors to stderr whenever possible
>    qemu_passt: Deduplicate passt killing code
>    qemu_passt: Let passt write the PID file

This is everything that was in the patch I sent last week, with the 
following additions

1) adding NULLSTR() around the reference to errbuf in patch 2/5

2) adding "--stderr" to the commandline in patch 3/5 (which I found to 
be unnecessary in my testing - as Stefano says everything goes to stderr 
until passt has completed its init anyway)

3) the other bit of patch 3/5 which adds an extra message telling the 
user to look into the designated logfile for the error - this is 
unnecessary (and actually now counter-productive, as it forces you to 
look elsewhere for the error when you wouldn't have needed to) because 
of patches I've sent to passt.

4)  patch 4/5 that is a cleanup de-duplicating code

5) patch 5 changes additional code (that I didn't touch in my patch) to 
use virPidFileReadPath() instead of virPidFileReadPathIfLocked(), and 
virProcessKillPainfully() instead of the higher level 
virPidFileForceCleanupPath().

So it all seems fine (except the error reporting stuff), but why revert 
a patch only to push back the same changes in a deconstructed fashion 
plus some fixups, rather than just posting a followup or two?

> 
>   src/qemu/qemu_passt.c | 61 +++++++++++++++++++++++++++++--------------
>   1 file changed, 41 insertions(+), 20 deletions(-)
> 

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Stefano Brivio]

On Thu, 16 Feb 2023 17:27:11 +0100
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 2/16/23 17:07, Stefano Brivio wrote:
> > On Thu, 16 Feb 2023 14:32:50 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> Passt has '--stderr' argument which makes it report error onto
> >> stderr rather to system log. Unfortunately, it's currently
> >> impossible to use both '--log-file' and '--stderr', so pass the
> >> latter only if the former isn't passed. Then, use the stderr to
> >> produce more user friendly error message on failed start.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
> >>  1 file changed, 19 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> >> index c082c149cd..881205449b 100644
> >> --- a/src/qemu/qemu_passt.c
> >> +++ b/src/qemu/qemu_passt.c
> >> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
> >>      if (net->sourceDev)
> >>          virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
> >>  
> >> -    if (net->backend.logFile)
> >> +    if (net->backend.logFile) {
> >>          virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
> >> +    } else {
> >> +        /* By default, passt logs into system logger. But we are interested
> >> +         * into errors too. Make it print errors onto stderr. */
> >> +        virCommandAddArg(cmd, "--stderr");
> >> +    }  
> > 
> > There's no need for this, see my previous email, archived at:
> >   https://listman.redhat.com/archives/libvir-list/2023-February/237880.html
> >   
> >>  
> >>      /* Add IP address info */
> >>      for (i = 0; i < net->guestIP.nips; i++) {
> >> @@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
> >>          goto error;
> >>  
> >>      if (cmdret < 0 || exitstatus != 0) {
> >> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> >> -                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
> >> +        if (STRNEQ_NULLABLE(errbuf, "")) {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Could not start 'passt': %s"), errbuf);
> >> +        } else if (net->backend.logFile) {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Could not start 'passt': look into %s for error"),
> >> +                           net->backend.logFile);  
> > 
> > ...and this won't be needed either, with Laine's series for passt. It
> > might actually be a bit misleading.
> >   
> >> +        } else {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Could not start 'passt': exit status = '%d'"),
> >> +                           exitstatus);
> >> +        }
> >> +
> >>          goto error;
> >>      }
> >>    
> > 
> > So all in all I think this is unnecessary, but also kind of harmless.
> >   
> 
> Except those patches are not merged yet.

Merged.

> And as we are getting close to
> the release I'd like to make this work with what we have now. We've been
> burned plenty of times with QEMU to learn our lesson. We've merged
> patches that were based not on QEMU's git, but some patches on top
> thinking - yeah, the API won't change. And then it did.
> 
> Now we don't require a release (which would be ideal), but at least for
> patches to be merged. When they get merged then yeah, this can be reworked.

-- 
Stefano

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[Laine Stump]

On 2/16/23 11:27 AM, Michal Prívozník wrote:
> On 2/16/23 17:07, Stefano Brivio wrote:
>> On Thu, 16 Feb 2023 14:32:50 +0100
>> Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>>> Passt has '--stderr' argument which makes it report error onto
>>> stderr rather to system log. Unfortunately, it's currently
>>> impossible to use both '--log-file' and '--stderr', so pass the
>>> latter only if the former isn't passed. Then, use the stderr to
>>> produce more user friendly error message on failed start.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>   src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
>>> index c082c149cd..881205449b 100644
>>> --- a/src/qemu/qemu_passt.c
>>> +++ b/src/qemu/qemu_passt.c
>>> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
>>>       if (net->sourceDev)
>>>           virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
>>>   
>>> -    if (net->backend.logFile)
>>> +    if (net->backend.logFile) {
>>>           virCommandAddArgList(cmd, "--log-file", net->backend.logFile, NULL);
>>> +    } else {
>>> +        /* By default, passt logs into system logger. But we are interested
>>> +         * into errors too. Make it print errors onto stderr. */
>>> +        virCommandAddArg(cmd, "--stderr");
>>> +    }
>>
>> There's no need for this, see my previous email, archived at:
>>    https://listman.redhat.com/archives/libvir-list/2023-February/237880.html
>>
>>>   
>>>       /* Add IP address info */
>>>       for (i = 0; i < net->guestIP.nips; i++) {
>>> @@ -265,8 +270,19 @@ qemuPasstStart(virDomainObj *vm,
>>>           goto error;
>>>   
>>>       if (cmdret < 0 || exitstatus != 0) {
>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
>>> +        if (STRNEQ_NULLABLE(errbuf, "")) {
>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                           _("Could not start 'passt': %s"), errbuf);
>>> +        } else if (net->backend.logFile) {
>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                           _("Could not start 'passt': look into %s for error"),
>>> +                           net->backend.logFile);
>>
>> ...and this won't be needed either, with Laine's series for passt. It
>> might actually be a bit misleading.
>>
>>> +        } else {
>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                           _("Could not start 'passt': exit status = '%d'"),
>>> +                           exitstatus);
>>> +        }
>>> +
>>>           goto error;
>>>       }
>>>   
>>
>> So all in all I think this is unnecessary, but also kind of harmless.
>>
> 
> Except those patches are not merged yet. And as we are getting close to
> the release I'd like to make this work with what we have now. We've been
> burned plenty of times with QEMU to learn our lesson. We've merged
> patches that were based not on QEMU's git, but some patches on top
> thinking - yeah, the API won't change. And then it did.

The patches are in passt, not QEMU, and Stefano will be cutting a new 
passt release "within a day or two".

> 
> Now we don't require a release (which would be ideal), but at least for
> patches to be merged. When they get merged then yeah, this can be reworked.

Re: [PATCH v2 2/5] qemu_passt: Report passt's error on failed start

[Laine Stump]

On 2/16/23 8:32 AM, Michal Privoznik wrote:
> When starting passt, it may write something onto its stderr
> (convincing it to print even more is addressed later). Pass this
> string we read to user.
> 
> Since we're not daemonizing passt anymore (see previous commit),
> we can let virCommand module do all the heavy lifting and switch
> to virCommandSetErrorBuffer() instead of reading error from an
> FD.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

(also a part of the reverted patch)

> ---
>   src/qemu/qemu_passt.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index adc69fc052..c082c149cd 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -144,18 +144,18 @@ qemuPasstStart(virDomainObj *vm,
>       g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
>       g_autoptr(virCommand) cmd = NULL;
>       g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> +    g_autofree char *errbuf = NULL;
>       char macaddr[VIR_MAC_STRING_BUFLEN];
>       size_t i;
>       pid_t pid = (pid_t) -1;
>       int exitstatus = 0;
>       int cmdret = 0;
> -    VIR_AUTOCLOSE errfd = -1;
>   
>       cmd = virCommandNew(PASST);
>   
>       virCommandClearCaps(cmd);
>       virCommandSetPidFile(cmd, pidfile);
> -    virCommandSetErrorFD(cmd, &errfd);
> +    virCommandSetErrorBuffer(cmd, &errbuf);
>   
>       virCommandAddArgList(cmd,
>                            "--one-off",
> @@ -266,7 +266,7 @@ qemuPasstStart(virDomainObj *vm,
>   
>       if (cmdret < 0 || exitstatus != 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Could not start 'passt'. exitstatus: %d"), exitstatus);
> +                       _("Could not start 'passt': %s"), NULLSTR(errbuf));
>           goto error;
>       }
>   

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

[Michal Prívozník]

On 2/16/23 17:07, Stefano Brivio wrote:
> On Thu, 16 Feb 2023 14:32:51 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> There are two places where we kill passt:
>>
>> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
>> 2) qemuPasstStart() - after failed start.
>>
>> Now, the code from 2) lack error preservation (so if there's
>> another error during cleanup we might overwrite the original
>> error). Therefore, move the internals of qemuPasstStop() into a
>> separate function and call it from both places.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_passt.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
>> index 881205449b..a4cc9e7166 100644
>> --- a/src/qemu/qemu_passt.c
>> +++ b/src/qemu/qemu_passt.c
>> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
>>  }
>>  
>>  
>> -void
>> -qemuPasstStop(virDomainObj *vm,
>> -              virDomainNetDef *net)
>> +static void
>> +qemuPasstKill(const char *pidfile)
> 
> A minor comment, should you respin: I think it should be made clear that
> this is not the expected/normal way in which passt will terminate --
> here or in the next patch. Removing the PID file is nice, but that's
> (usually) about it.


I can adjust the commit message.

> 
>>  {
>> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>>      virErrorPtr orig_err;
>>  
>>      virErrorPreserveLast(&orig_err);
>> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
>>  }
>>  
>>  
>> +void
>> +qemuPasstStop(virDomainObj *vm,
>> +              virDomainNetDef *net)
>> +{
>> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>> +
>> +    qemuPasstKill(pidfile);
>> +}
>> +
>> +
>>  int
>>  qemuPasstSetupCgroup(virDomainObj *vm,
>>                       virDomainNetDef *net,
>> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
>>      g_autofree char *errbuf = NULL;
>>      char macaddr[VIR_MAC_STRING_BUFLEN];
>>      size_t i;
>> -    pid_t pid = (pid_t) -1;
>>      int exitstatus = 0;
>>      int cmdret = 0;
>>  
>> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
>>      return 0;
>>  
>>   error:
>> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
>> -    if (pid != -1)
>> -        virProcessKillPainfully(pid, true);
>> -    unlink(pidfile);
>> -
>> +    qemuPasstKill(pidfile);
> 
> ...what takes care of terminating passt in case qemu doesn't start, now?
> The fact that the process is in the cgroup, right?

No, it's qemuPasstKill(). Starting a guest is done in
qemuProcessStart(). In here, qemuProcessLaunch() ->
qemuExtDevicesStart() -> qemuPasstStart() is called. Now, in the top
most parent (qemuProcessStart()) - you can see the 'stop' label in which
qemuProcessStop() -> qemuExtDevicesStop() -> qemuPasstStop() is called.

We could go the extra step and use either virCgroupKillRecursive() or
virCgroupKillPainfully() to kill all processes within the CGroup. BUT,
the driver runs on different OSes than Linux (e.g. FreeBSD) or on Linux
with no CGroups.

Michal

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

[Laine Stump]

On 2/16/23 8:32 AM, Michal Privoznik wrote:
> There are two places where we kill passt:
> 
> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
> 2) qemuPasstStart() - after failed start.
> 
> Now, the code from 2) lack error preservation (so if there's
> another error during cleanup we might overwrite the original
> error). Therefore, move the internals of qemuPasstStop() into a
> separate function and call it from both places.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

> ---
>   src/qemu/qemu_passt.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index 881205449b..a4cc9e7166 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
>   }
>   
>   
> -void
> -qemuPasstStop(virDomainObj *vm,
> -              virDomainNetDef *net)
> +static void
> +qemuPasstKill(const char *pidfile)
>   {
> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>       virErrorPtr orig_err;
>   
>       virErrorPreserveLast(&orig_err);
> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
>   }
>   
>   
> +void
> +qemuPasstStop(virDomainObj *vm,
> +              virDomainNetDef *net)
> +{
> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> +
> +    qemuPasstKill(pidfile);
> +}
> +
> +
>   int
>   qemuPasstSetupCgroup(virDomainObj *vm,
>                        virDomainNetDef *net,
> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
>       g_autofree char *errbuf = NULL;
>       char macaddr[VIR_MAC_STRING_BUFLEN];
>       size_t i;
> -    pid_t pid = (pid_t) -1;
>       int exitstatus = 0;
>       int cmdret = 0;
>   
> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
>       return 0;
>   
>    error:
> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
> -    if (pid != -1)
> -        virProcessKillPainfully(pid, true);
> -    unlink(pidfile);
> -
> +    qemuPasstKill(pidfile);
>       return -1;
>   }

Re: [PATCH v2 5/5] qemu_passt: Let passt write the PID file

[Laine Stump]

On 2/16/23 8:32 AM, Michal Privoznik wrote:
> The way we start passt currently is: we use
> virCommandSetPidFile() to use our virCommand machinery to acquire
> the PID file and leak opened FD into passt. Then, we use
> virPidFile*() APIs to read the PID file (which is needed when
> placing it into CGroups or killing it). But this does not fly
> really because passt daemonizes itself. Thus the process we
> started dies soon and thus the PID file is closed and unlocked.
> 
> We could work around this by passing '--foreground' argument, but
> that weakens passt as it can't create new PID namespace (because
> it doesn't fork()).
> 
> The solution is to let passt write the PID file, but since it
> does not lock the file and closes it as soon as it is written, we
> have to switch to those virPidFile APIs which don't expect PID
> file to be locked.

So *this* is the part that was functionally wrong after my earlier patch 
was applied - I had switched to using an externally-generated pidfile, 
but was still using the APIs that should have only been used for 
pidfiles created by libvirt.

(You had mentioned some sort of cgroup issue last week. Did that solve 
itself? I never saw the problem, and passt has been starting/stopping 
fine for me all along (both before and after I changed the daemonizing) 
as long as selinux is disabled - still need to fix that).

> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

> ---
>   src/qemu/qemu_passt.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> index a4cc9e7166..47f4b5fcae 100644
> --- a/src/qemu/qemu_passt.c
> +++ b/src/qemu/qemu_passt.c
> @@ -72,7 +72,7 @@ qemuPasstGetPid(virDomainObj *vm,
>   {
>       g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
>   
> -    return virPidFileReadPathIfLocked(pidfile, pid);
> +    return virPidFileReadPath(pidfile, pid);
>   }
>   
>   
> @@ -106,11 +106,14 @@ static void
>   qemuPasstKill(const char *pidfile)
>   {
>       virErrorPtr orig_err;
> +    pid_t pid = 0;
>   
>       virErrorPreserveLast(&orig_err);
>   
> -    if (virPidFileForceCleanupPath(pidfile) < 0)
> -        VIR_WARN("Unable to kill passt process");
> +    ignore_value(virPidFileReadPath(pidfile, &pid));
> +    if (pid != 0)
> +        virProcessKillPainfully(pid, true);
> +    unlink(pidfile);
>   
>       virErrorRestore(&orig_err);
>   }
> @@ -161,13 +164,13 @@ qemuPasstStart(virDomainObj *vm,
>       cmd = virCommandNew(PASST);
>   
>       virCommandClearCaps(cmd);
> -    virCommandSetPidFile(cmd, pidfile);
>       virCommandSetErrorBuffer(cmd, &errbuf);
>   
>       virCommandAddArgList(cmd,
>                            "--one-off",
>                            "--socket", passtSocketName,
>                            "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
> +                         "--pid", pidfile,
>                            NULL);
>   
>       if (net->mtu) {

Re: [PATCH v2 4/5] qemu_passt: Deduplicate passt killing code

[Stefano Brivio]

On Thu, 16 Feb 2023 17:38:47 +0100
Michal Prívozník <mprivozn@redhat.com> wrote:

> On 2/16/23 17:07, Stefano Brivio wrote:
> > On Thu, 16 Feb 2023 14:32:51 +0100
> > Michal Privoznik <mprivozn@redhat.com> wrote:
> >   
> >> There are two places where we kill passt:
> >>
> >> 1) qemuPasstStop() - called transitively from qemuProcessStop(),
> >> 2) qemuPasstStart() - after failed start.
> >>
> >> Now, the code from 2) lack error preservation (so if there's
> >> another error during cleanup we might overwrite the original
> >> error). Therefore, move the internals of qemuPasstStop() into a
> >> separate function and call it from both places.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_passt.c | 23 +++++++++++++----------
> >>  1 file changed, 13 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> >> index 881205449b..a4cc9e7166 100644
> >> --- a/src/qemu/qemu_passt.c
> >> +++ b/src/qemu/qemu_passt.c
> >> @@ -102,11 +102,9 @@ qemuPasstAddNetProps(virDomainObj *vm,
> >>  }
> >>  
> >>  
> >> -void
> >> -qemuPasstStop(virDomainObj *vm,
> >> -              virDomainNetDef *net)
> >> +static void
> >> +qemuPasstKill(const char *pidfile)  
> > 
> > A minor comment, should you respin: I think it should be made clear that
> > this is not the expected/normal way in which passt will terminate --
> > here or in the next patch. Removing the PID file is nice, but that's
> > (usually) about it.  
> 
> I can adjust the commit message.

Okay, I'm not really familiar with libvirt's code, so I don't know how
appropriate this is -- I was just suggesting that a _comment_ to a
qemuPasstKill() function which does *not* actually kill the passt
process, unless an error occurs, wouldn't look so bizarre.

> >>  {
> >> -    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> >>      virErrorPtr orig_err;
> >>  
> >>      virErrorPreserveLast(&orig_err);
> >> @@ -118,6 +116,16 @@ qemuPasstStop(virDomainObj *vm,
> >>  }
> >>  
> >>  
> >> +void
> >> +qemuPasstStop(virDomainObj *vm,
> >> +              virDomainNetDef *net)
> >> +{
> >> +    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
> >> +
> >> +    qemuPasstKill(pidfile);
> >> +}
> >> +
> >> +
> >>  int
> >>  qemuPasstSetupCgroup(virDomainObj *vm,
> >>                       virDomainNetDef *net,
> >> @@ -147,7 +155,6 @@ qemuPasstStart(virDomainObj *vm,
> >>      g_autofree char *errbuf = NULL;
> >>      char macaddr[VIR_MAC_STRING_BUFLEN];
> >>      size_t i;
> >> -    pid_t pid = (pid_t) -1;
> >>      int exitstatus = 0;
> >>      int cmdret = 0;
> >>  
> >> @@ -289,10 +296,6 @@ qemuPasstStart(virDomainObj *vm,
> >>      return 0;
> >>  
> >>   error:
> >> -    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
> >> -    if (pid != -1)
> >> -        virProcessKillPainfully(pid, true);
> >> -    unlink(pidfile);
> >> -
> >> +    qemuPasstKill(pidfile);  
> > 
> > ...what takes care of terminating passt in case qemu doesn't start, now?
> > The fact that the process is in the cgroup, right?  
> 
> No, it's qemuPasstKill(). Starting a guest is done in
> qemuProcessStart(). In here, qemuProcessLaunch() ->
> qemuExtDevicesStart() -> qemuPasstStart() is called. Now, in the top
> most parent (qemuProcessStart()) - you can see the 'stop' label in which
> qemuProcessStop() -> qemuExtDevicesStop() -> qemuPasstStop() is called.

Ah, okay, thanks for the explanation, I see now (well, it makes
sense with 5/5).

-- 
Stefano

Re: [PATCH v2 3/5] qemu_passt: Make passt report errors to stderr whenever possible

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

On Thu, Feb 16, 2023 at 05:21:48PM +0100, Michal Prívozník wrote:
> On 2/16/23 16:42, Jonathon Jongsma wrote:
> > On 2/16/23 7:32 AM, Michal Privoznik wrote:
> >> Passt has '--stderr' argument which makes it report error onto
> >> stderr rather to system log. Unfortunately, it's currently
> >> impossible to use both '--log-file' and '--stderr', so pass the
> >> latter only if the former isn't passed. Then, use the stderr to
> >> produce more user friendly error message on failed start.
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>   src/qemu/qemu_passt.c | 22 +++++++++++++++++++---
> >>   1 file changed, 19 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
> >> index c082c149cd..881205449b 100644
> >> --- a/src/qemu/qemu_passt.c
> >> +++ b/src/qemu/qemu_passt.c
> >> @@ -171,8 +171,13 @@ qemuPasstStart(virDomainObj *vm,
> >>       if (net->sourceDev)
> >>           virCommandAddArgList(cmd, "--interface", net->sourceDev, NULL);
> >>   -    if (net->backend.logFile)
> >> +    if (net->backend.logFile) {
> >>           virCommandAddArgList(cmd, "--log-file",
> >> net->backend.logFile, NULL);
> >> +    } else {
> >> +        /* By default, passt logs into system logger. But we are
> >> interested
> >> +         * into errors too. Make it print errors onto stderr. */
> > 
> > s/into/in the/ ?
> 
> Honestly, I have no idea. I'm not a native speaker.

I am, and "in the" definitely seems more correct here.

> Maybe it's 'print
> onto paper' but 'print into a stream'?

Fwiw, s/onto/to/ also reads slightly better.

> 
> Anyway, fixed locally. Thanks.
> 
> Michal
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/5] qemu_passt: Fix issues with PID file

[Michal Prívozník]

On 2/16/23 17:35, Laine Stump wrote:
> On 2/16/23 8:32 AM, Michal Privoznik wrote:
>> This is a v2 of:
>>
>> https://listman.redhat.com/archives/libvir-list/2023-February/237731.html
>>
>> diff to v1:
>> - Merged patches that were ACKed in v1,
>> - Dropped 4/4 from the original series (the one that sets --foreground),
>>    and implemented a different approach
>>
>> Michal Prívozník (5):
>>    qemu_passt: Avoid double daemonizing passt
>>    qemu_passt: Report passt's error on failed start
>>    qemu_passt: Make passt report errors to stderr whenever possible
>>    qemu_passt: Deduplicate passt killing code
>>    qemu_passt: Let passt write the PID file
> 
> This is everything that was in the patch I sent last week, with the
> following additions
> 
> 1) adding NULLSTR() around the reference to errbuf in patch 2/5
> 
> 2) adding "--stderr" to the commandline in patch 3/5 (which I found to
> be unnecessary in my testing - as Stefano says everything goes to stderr
> until passt has completed its init anyway)
> 
> 3) the other bit of patch 3/5 which adds an extra message telling the
> user to look into the designated logfile for the error - this is
> unnecessary (and actually now counter-productive, as it forces you to
> look elsewhere for the error when you wouldn't have needed to) because
> of patches I've sent to passt.
> 
> 4)  patch 4/5 that is a cleanup de-duplicating code
> 
> 5) patch 5 changes additional code (that I didn't touch in my patch) to
> use virPidFileReadPath() instead of virPidFileReadPathIfLocked(), and
> virProcessKillPainfully() instead of the higher level
> virPidFileForceCleanupPath().
> 
> So it all seems fine (except the error reporting stuff), but why revert
> a patch only to push back the same changes in a deconstructed fashion
> plus some fixups, rather than just posting a followup or two?

Yeah, I realized this too and I'm sorry. My original intention was to
fix this in a completely different way (as my last patch from v1
demonstrates) and that was incompatible with yours.

Michal