| Message ID | 20250121100029.1106973-1-lvivier@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user: Silence unsupported VHOST_USER_PROTOCOL_F_RARP error | expand |
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
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
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 >
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 > >
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/
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
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
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
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);
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(-)