Re: [PATCH 3/7] vhost-user: factor out "vhost_user_write_msg"

[Laszlo Ersek <lersek@redhat.com>]

On 8/30/23 11:14, Laszlo Ersek wrote:
> On 8/30/23 10:31, Stefano Garzarella wrote:
>> On Sun, Aug 27, 2023 at 08:29:33PM +0200, Laszlo Ersek wrote:
>>> The tails of the "vhost_user_set_vring_addr" and "vhost_user_set_u64"
>>> functions are now byte-for-byte identical. Factor the common tail out
>>> to a
>>> new function called "vhost_user_write_msg".
>>>
>>> This is purely refactoring -- no observable change.
>>>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> (supporter:vhost)
>>> Cc: Eugenio Perez Martin <eperezma@redhat.com>
>>> Cc: German Maglione <gmaglione@redhat.com>
>>> Cc: Liu Jiang <gerry@linux.alibaba.com>
>>> Cc: Sergio Lopez Pascual <slp@redhat.com>
>>> Cc: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>> hw/virtio/vhost-user.c | 66 +++++++++-----------
>>> 1 file changed, 28 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>> index 64eac317bfb2..36f99b66a644 100644
>>> --- a/hw/virtio/vhost-user.c
>>> +++ b/hw/virtio/vhost-user.c
>>> @@ -1320,10 +1320,35 @@ static int enforce_reply(struct vhost_dev *dev,
>>>     return vhost_user_get_features(dev, &dummy);
>>> }
>>>
>>> +/* Note: "msg->hdr.flags" may be modified. */
>>> +static int vhost_user_write_msg(struct vhost_dev *dev, VhostUserMsg
>>> *msg,
>>> +                                bool wait_for_reply)
>>
>> The difference between vhost_user_write() and vhost_user_write_msg() is
>> not immediately obvious from the function name, so I would propose
>> something different, like vhost_user_write_sync() or
>> vhost_user_write_wait().
> 
> I'm mostly OK with either variant; I think I may have thought of _sync
> myself, but didn't like it because the wait would be *optional*,
> dependent on caller choice. And I didn't like
> vhost_user_write_maybe_wait() either; that one seemed awkward / too verbose.
> 
> Let's see what others prefer. :)

... I went with vhost_user_write_sync.

> 
>>
>> Anyway, I'm not good with names and don't have a strong opinion, so this
>> version is fine with me as well :-)
>>
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>
> 
> Thanks!