Re: [PATCH v9 2/5] generic vhost user server

[Coiby Xu <coiby.xu@gmail.com>]

On Fri, Jun 19, 2020 at 01:13:00PM +0100, Stefan Hajnoczi wrote:
>On Mon, Jun 15, 2020 at 02:39:04AM +0800, Coiby Xu wrote:
>> +/*
>> + * a wrapper for vu_kick_cb
>> + *
>> + * since aio_dispatch can only pass one user data pointer to the
>> + * callback function, pack VuDev and pvt into a struct. Then unpack it
>> + * and pass them to vu_kick_cb
>> + */
>> +static void kick_handler(void *opaque)
>> +{
>> +    KickInfo *kick_info = opaque;
>> +    kick_info->cb(kick_info->vu_dev, 0, (void *) kick_info->index);
>
>Where is kick_info->index assigned? It appears to be NULL in all cases.
>
>> +}
>> +
>> +
>> +static void
>> +set_watch(VuDev *vu_dev, int fd, int vu_evt,
>> +          vu_watch_cb cb, void *pvt)
>> +{
>> +
>> +    VuServer *server = container_of(vu_dev, VuServer, vu_dev);
>> +    g_assert(vu_dev);
>> +    g_assert(fd >= 0);
>> +    long index = (intptr_t) pvt;
>
>The meaning of the pvt argument is not defined in the library interface.
>set_watch() callbacks shouldn't interpret pvt.
>
>You could modify libvhost-user to explicitly pass the virtqueue index
>(or -1 if the fd is not associated with a virtqueue), but it's nice to
>avoid libvhost-user API changes so that existing libvhost-user
>applications don't require modifications.
>
>What I would do here is to change the ->kick_info[] data struct. How
>about a linked list of VuFdWatch objects? That way the code can handle
>any number of fd watches and doesn't make assumptions about virtqueues.
>set_watch() is a generic fd monitoring interface and doesn't need to be
>tied to virtqueues.

A linked list of VuFdWatch objects has been adopted in v10. Thank you!

--
Best regards,
Coiby