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

["Michal Prívozník" <mprivozn@redhat.com>]

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