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

[Thomas Monjalon <thomas@monjalon.net>]

01/03/2018 07:02, Tan, Jianfeng:
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> > On 02/28/2018 02:36 AM, Yang, Zhiyong wrote:
> > > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> > >> On 02/14/2018 03:53 PM, Zhiyong Yang wrote:
> > >>>    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.
> 
> I vote to move it to generic library, maybe eal. Poll() has better compatibility even though poll() is not as performant as epoll().
> 
> Thomas, how do you think?

I don't see why it should be exported outside of DPDK, except for PMDs.
I would tend to keep it internal but I understand that it would mean
duplicating some code, which is not ideal.
Please could you show what would be the content of the .h in EAL?