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

[Laine Stump <laine@redhat.com>]

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.