> -----Original Message----- > From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] > Sent: Wednesday, February 28, 2018 1:52 AM > To: Yang, Zhiyong <zhiyong.y...@intel.com>; dev@dpdk.org; > y...@fridaylinux.org; Tan, Jianfeng <jianfeng....@intel.com>; Bie, Tiwei > <tiwei....@intel.com>; Wang, Zhihong <zhihong.w...@intel.com> > Cc: Wang, Dong1 <dong1.w...@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.y...@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. 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? thanks Zhiyong
