On 02/28/2018 02:36 AM, Yang, Zhiyong wrote:
-----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.
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
