Re: [PATCH 1/4] vhost: move fdset functions from fd_man.c to fd_man.h

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 02/28/2018 02:36 AM, Yang, Zhiyong wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Wednesday, February 28, 2018 1:52 AM
>> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org;
>> yliu@fridaylinux.org; Tan, Jianfeng <jianfeng.tan@intel.com>; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
>> Cc: Wang, Dong1 <dong1.wang@intel.com>
>> Subject: Re: [PATCH 1/4] vhost: move fdset functions from fd_man.c to
>> fd_man.h
>>
>> Hi Zhiyong,
>>
>> On 02/14/2018 03:53 PM, Zhiyong Yang wrote:
>>> The patch moves fdset related funcitons from fd_man.c to fd_man.h in
>>> order to reuse these funcitons from the perspective of PMDs.
>>>
>>> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
>>> ---
>>>    lib/librte_vhost/Makefile |   3 +-
>>>    lib/librte_vhost/fd_man.c | 274 ----------------------------------------------
>>>    lib/librte_vhost/fd_man.h | 258
>> +++++++++++++++++++++++++++++++++++++++++--
>>>    3 files changed, 253 insertions(+), 282 deletions(-)
>>>    delete mode 100644 lib/librte_vhost/fd_man.c
>>
>> I disagree with the patch.
>> It is a good thing to reuse the code, but to do it, you need to extend the
>> vhost lib API.
>>
>> New API need to be prefixed with rte_vhost_, and be declared in
>> rte_vhost.h.
>>
>> And no need to move the functions from the .c to the .h file, as it moreover
>> makes you inline them, which is not necessary here.
> 
> Thanks for your reviewing the series firstly, Maxime. :)
> 
> I considered to do it as you said. However I still preferred this one at last.
> Here are my reasons.
> 1) As far as I know, this set of functions are used privately in librte_vhost before this feature.
> No strong request from the perspective of DPDK application. If I understand well,  It is enough to expose the functions to all PMDs
> And it is better to keep internal use in DPDK.

But what the patch is doing is adding fd_man.h to the API, without doing
it properly. fd_man.h will be installed with other header files, and any
external application can use it.

> 
> 2) These functions help to implement vhost user, but they are not strongly related to other APIs of vhost user which have already exposed.
> if we want to expose them as APIs at lib layer, many functions and related data structure has to be exposed in rte_vhost.h. it looks messy.
> Your opinion?

Yes, it is not really vhost-related, it could be part of a more generic
library. It is maybe better to duplicate these lines, or to move this
code in a existing or new library.

Cheers,
Maxime

> thanks
> Zhiyong
>