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

[Stefano Brivio]

[Cc'ed Thibaut as author of 3e866365e1eb ("vhost user: add rarp sending
after live migration for legacy guest")]

On Wed, 22 Jan 2025 17:51:07 +0100
Stefano Garzarella <sgarzare@redhat.com> 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?

By the way, we don't need that in passt because we don't need to update
any ARP table. Even if the guest changes its MAC address, with passt,
nobody is going to notice.

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

By the way, I think that that message is inaccurate on a number of
other levels:

- if the back-end doesn't even try, it can't fail, and "fails"
  indicates... failure?

- RARP is an obsolete protocol, but there's no such thing as a fake
  protocol. It's not even a fake message, it's a simple RARP broadcast
  message, with the correct MAC address. It's "dummy", at most

- I read the whole thread but still I can't understand why we have a
  capability then: what does it (or its absence) mean?

Regardless of that, sure, let's go ahead and add the fake (that one
*is* fake) callback in passt, which Laurent just sent a patch for. We
need it anyway as a workaround for compatibility with current/older
versions of QEMU.

But I don't understand why we're leaving this as it is.

-- 
Stefano