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

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; }

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; }

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

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

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.

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

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; }

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) {