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(a)redhat.com&gt; --- 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",

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(a)redhat.com&gt;

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

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(a)redhat.com&gt; --- 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; }

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(a)redhat.com&gt; >> --- >>   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

On Thu, 16 Feb 2023 14:32:50 +0100 Michal Privoznik <mprivozn(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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.

On 2/16/23 17:07, Stefano Brivio wrote: > On Thu, 16 Feb 2023 14:32:50 +0100 > Michal Privoznik <mprivozn(a)redhat.com&gt; 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(a)redhat.com&gt; >> --- >> 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.

On Thu, 16 Feb 2023 14:32:51 +0100 Michal Privoznik <mprivozn(a)redhat.com&gt; 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(a)redhat.com&gt; > --- > 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?

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(a)redhat.com&gt;

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

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(a)redhat.com&gt;

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