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

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

...
  On 24/01/2025 19:40, Stefano Brivio wrote:
  On Fri, 24 Jan 2025 19:35:33 +0100
 Laurent Vivier <lvivier(a)redhat.com> wrote:
   
  On 24/01/2025 17:10, Stefano Brivio wrote:  
  On Fri, 24 Jan 2025 15:21:37 +0100
 Laurent Vivier <lvivier(a)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(a)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.…

 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