On Thu, 16 Feb 2023 17:27:11 +0100 Michal Prívozník <mpriv...@redhat.com> wrote:
> On 2/16/23 17:07, Stefano Brivio wrote: > > On Thu, 16 Feb 2023 14:32:50 +0100 > > Michal Privoznik <mpriv...@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 <mpriv...@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