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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jan 22, 2025 at 06:22:06PM +0100, Laurent Vivier wrote:
> 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)?

If it is valid to interpret "VHOST_USER_SEND_RARP" to mean the service
is "capable of sending a RARP where appropriate" rather than "will always
unconditionally send an RARP", then it would be reasonable for Passt to
have an empty VHOST_USER_SEND_RARP impl.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|