Re: [PATCH] vhost-user: Implement an empty VHOST_USER_SEND_RARP command

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 24 Jan 2025 19:50:57 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 24/01/2025 19:40, Stefano Brivio wrote:
> > On Fri, 24 Jan 2025 19:35:33 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> On 24/01/2025 17:10, Stefano Brivio wrote:  
> >>> On Fri, 24 Jan 2025 15:21:37 +0100
> >>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>      
> >>>> Passt cannot manage and doesn't need to manage the broadcast of a fake RARP,
> >>>> but QEMU will report an error message if Passt doesn't implement it.
> >>>>
> >>>> Implement an empty SEND_RARP command to silence QEMU error message.
> >>>>
> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >>>> ---
> >>>>    vhost_user.c | 28 +++++++++++++++++++++++++++-
> >>>>    1 file changed, 27 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/vhost_user.c b/vhost_user.c
> >>>> index f12dec5ddc58..e6633ae75ce8 100644
> >>>> --- a/vhost_user.c
> >>>> +++ b/vhost_user.c
> >>>> @@ -914,7 +914,8 @@ static bool vu_get_protocol_features_exec(struct vu_dev *vdev,
> >>>>    {
> >>>>    	uint64_t features = 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |
> >>>>    			    1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD |
> >>>> -			    1ULL << VHOST_USER_PROTOCOL_F_DEVICE_STATE;
> >>>> +			    1ULL << VHOST_USER_PROTOCOL_F_DEVICE_STATE |
> >>>> +			    1ULL << VHOST_USER_PROTOCOL_F_RARP;
> >>>>    
> >>>>    	(void)vdev;
> >>>>    	vmsg_set_reply_u64(msg, features);
> >>>> @@ -981,6 +982,30 @@ static bool vu_set_vring_enable_exec(struct vu_dev *vdev,
> >>>>    	return false;
> >>>>    }
> >>>>    
> >>>> +/**
> >>>> + * vu_set_send_rarp_exec() - Broadcast a fake RARP to notify the migration
> >>>> + *			     is terminated  
> >>>
> >>> Fine, so we need to add this.
> >>>
> >>> But can we at least make it clear for our future benefit? That is,
> >>> there's no such thing as "fake RARP". The only thing that's actually
> >>> fake here is this callback. For others, see thread at:
> >>>
> >>>     https://lore.kernel.org/qemu-devel/20250121100029.1106973-1-lvivier@redhat.com/
> >>>
> >>> What about "Do nothing to silence QEMU bogus error message"? Claiming
> >>> we are broadcasting a RARP message and not doing it is... confusing.  
> >>
> >> I think it's interesting to have this comment as it comes from the vhost-user
> >> specification as it describes the aim of the command, and we can add something like "but
> >> as passt don't need to update any ARP table we do nothing only to silence QEMU bogus error
> >> message".  
> > 
> > Eh, if we're already making it verbose, maybe we can go with something
> > like:
> > 
> > 	vhost-user specification says: "Broadcast ...", but passt ...  
> 
> As you prefer...
> 
> If you want you can update the patch when you commit it.

Given it's already two edits it would save me some time if you could
write/format it properly...

-- 
Stefano