Re: [PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[Laurent Vivier <lvivier@redhat.com>]

On 22/01/2025 17:51, Stefano Garzarella wrote:
> On Wed, Jan 22, 2025 at 05:41:15PM +0100, Laurent Vivier wrote:
>> On 22/01/2025 17:20, Stefano Garzarella wrote:
>>> On Wed, Jan 22, 2025 at 08:59:22AM -0500, Michael S. Tsirkin wrote:
>>>> On Wed, Jan 22, 2025 at 02:42:14PM +0100, Stefano Garzarella wrote:
>>>>> On Tue, Jan 21, 2025 at 11:00:29AM +0100, Laurent Vivier wrote:
>>>>>> In vhost_user_receive() if vhost_net_notify_migration_done() reports
>>>>>> an error we display on the console:
>>>>>>
>>>>>>   Vhost user backend fails to broadcast fake RARP
>>>>>>
>>>>>> This message can be useful if there is a problem to execute
>>>>>> VHOST_USER_SEND_RARP but it is useless if the backend doesn't
>>>>>> support VHOST_USER_PROTOCOL_F_RARP.
>>>>>>
>>>>>> Don't report the error if vhost_net_notify_migration_done()
>>>>>> returns -ENOTSUP (from vhost_user_migration_done())
>>>>>>
>>>>>> Update vhost_net-stub.c to return -ENOTSUP too.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>> ---
>>>>>> hw/net/vhost_net-stub.c | 2 +-
>>>>>> net/vhost-user.c        | 2 +-
>>>>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
>>>>>> index 72df6d757e4d..875cd6c2b9c8 100644
>>>>>> --- a/hw/net/vhost_net-stub.c
>>>>>> +++ b/hw/net/vhost_net-stub.c
>>>>>> @@ -93,7 +93,7 @@ void vhost_net_config_mask(VHostNetState 
>>>>> *net, VirtIODevice *dev, bool mask)
>>>>>>
>>>>>> int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
>>>>>> {
>>>>>> -    return -1;
>>>>>> +    return -ENOTSUP;
>>>>>> }
>>>>>>
>>>>>> VHostNetState *get_vhost_net(NetClientState *nc)
>>>>>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>>>>>> index 12555518e838..636fff8a84a2 100644
>>>>>> --- a/net/vhost-user.c
>>>>>> +++ b/net/vhost-user.c
>>>>>> @@ -146,7 +146,7 @@ static ssize_t 
>>>>> vhost_user_receive(NetClientState *nc, const uint8_t *buf,
>>>>>>
>>>>>>          r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
>>>>>>
>>>>>> -        if ((r != 0) && (display_rarp_failure)) {
>>>>>> +        if ((r != 0) && (r != -ENOTSUP) && (display_rarp_failure)) {
>>>>>>              fprintf(stderr,
>>>>>>                      "Vhost user backend fails to broadcast fake RARP\n");
>>>>>>              fflush(stderr);
>>>>>> -- 
>>>>>> 2.47.1
>>>>>>
>>>>>
>>>>> IIUC the message was there since the introduction about 10 years ago
>>>>> from commit 3e866365e1 ("vhost user: add rarp sending after live
>>>>> migration for legacy guest"). IIUC -ENOTSUP is returned when both F_RARP
>>>>> and F_GUEST_ANNOUNCE are not negotiated.
>>>>>
>>>>> That said, I honestly don't know what F_RARP or F_GUEST_ANNOUNCE is for,
>>>>
>>>> rarp is to have destination host broadcast a message with VM address
>>>> to update the network. Guest announce is when it will instead
>>>> ask the guest to do this.
>>>
>>> Okay, thanks for explaining to me.
>>> So if both features are not negotiated, no one is going to broadcast
>>> the message, right?
>>>
>>> Could that be a valid reason to print an error message in QEMU?
>>>
>>> To me it might be reasonable because the user might experience some
>>> network problems, but I'm not a network guy :-)
>>
>> I'm working on adding vhost-user to passt[1], and in this case we don't need to 
>> broadcast any message.
> 
> Okay, so please can you add that to the commit description and also
> explaining why you don't need that?
> 
>>
>> So I don't implement VHOST_USER_SEND_RARP and I don't want the error message to spoil my 
>> console.
> 
> Fair enough, but at that point, if it's valid to have both feature not
> negotiated, IMHO is better to return 0 in vhost_user_migration_done().
> Maybe adding also a comment to explain that in your scenario you don't
> need to do nothing (like if guest supports GUEST_ANNOUNCE).

I agree.

> 
>>
>> -ENOTSUP is an error message for developer not for user.
> 
> I was referring to the "Vhost user backend fails to broadcast fake RARP"
> error message we are skipping here.

So in the end my real question at this point is:

is it better to suppress the error message in QEMU (1) or implement an empty 
VHOST_USER_SEND_RARP in Passt (2)?

As this discussion is going on I'm no longer sure that (1) is the right one.

Thanks,
Laurent