vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

[PATCH] vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error

Posted by Laurent Vivier 1 day, 13 hours ago

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

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

Posted by Stefano Garzarella 10 hours ago

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,
but my understanding is that the message was to notify that the
migration was finished (reading that commit).

If neither feature is supported, could this be a problem for the user
and that's why we were printing the message?

Thanks,
Stefano

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

Posted by Michael S. Tsirkin 9 hours ago

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.


> but my understanding is that the message was to notify that the
> migration was finished (reading that commit).
> 
> If neither feature is supported, could this be a problem for the user
> and that's why we were printing the message?
> 
> Thanks,
> Stefano

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

Posted by Stefano Garzarella 7 hours ago

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

Thanks,
Stefano

>
>
>> but my understanding is that the message was to notify that the
>> migration was finished (reading that commit).
>>
>> If neither feature is supported, could this be a problem for the user
>> and that's why we were printing the message?
>>
>> Thanks,
>> Stefano
>

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

Posted by Laurent Vivier 7 hours ago

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.

So I don't implement VHOST_USER_SEND_RARP and I don't want the error message to spoil my 
console.

-ENOTSUP is an error message for developer not for user.

Thanks,
Laurent
[1] https://passt.top/passt/

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

Posted by Stefano Garzarella 7 hours ago

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

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

Thanks,
Stefano

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

Posted by Laurent Vivier 6 hours ago

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

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

Posted by Daniel P. Berrangé 6 hours ago

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 :|

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

Posted by Michael S. Tsirkin 7 hours ago

On Wed, Jan 22, 2025 at 05:20:06PM +0100, 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 :-)
> 
> Thanks,
> Stefano

reasonable, yes.

> > 
> > 
> > > but my understanding is that the message was to notify that the
> > > migration was finished (reading that commit).
> > > 
> > > If neither feature is supported, could this be a problem for the user
> > > and that's why we were printing the message?
> > > 
> > > Thanks,
> > > Stefano
> >