Re: [dpdk-dev] [PATCH 1/4] kni: add function to query the name of a kni object

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, May 27, 2015 at 04:15:49PM +0200, Marc Sune wrote:
> 
> 
> On 27/05/15 15:55, Bruce Richardson wrote:
> >On Wed, May 27, 2015 at 03:52:34PM +0200, Marc Sune wrote:
> >>
> >>On 27/05/15 15:47, Bruce Richardson wrote:
> >>>When a KNI object is created, a name is assigned to it which is stored
> >>>internally. There is also an API function to look up a KNI object by
> >>>name, but there is no API to query the current name of an existing
> >>>KNI object. This patch adds just such an API.
> >>>
> >>>Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> >>>---
> >>>  lib/librte_kni/rte_kni.c           |  6 ++++++
> >>>  lib/librte_kni/rte_kni.h           | 10 ++++++++++
> >>>  lib/librte_kni/rte_kni_version.map |  1 +
> >>>  3 files changed, 17 insertions(+)
> >>>
> >>>diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> >>>index 4e70fa0..c5a0089 100644
> >>>--- a/lib/librte_kni/rte_kni.c
> >>>+++ b/lib/librte_kni/rte_kni.c
> >>>@@ -674,6 +674,12 @@ rte_kni_get(const char *name)
> >>>  	return NULL;
> >>>  }
> >>>+const char *
> >>>+rte_kni_get_name(const struct rte_kni *kni)
> >>>+{
> >>>+	return kni->name;
> >>>+}
> >>Since a pointer to the kni context (struct rte_kni) is exposed to the user
> >>(rte_kni_get() and rte_kni_alloc ()), and the field is directly in the
> >>struct, is this API call really necessary? I would only see this necessary
> >>if the API would only expose a handle, like a port_id for ethdev
> >>
> >>Marc
> >The structure definition is in rte_kni.c, not in the header file, so applications
> >can't read the name directly. In other words, the create API just exposes a handle.
> >[The structure in the header is the conf structure, not the full kni struct]
> 
> Ops, you are right. I overlooked that. What about:
> 
> extern void rte_kni_get_config(const struct rte_kni *kni, struct
> rte_kni_conf* conf);
> 
> which fills in (copies) the fields of conf would allow to recover the
> original configuration, including the name? It is closer
> rte_eth_dev_info_get (unfortunately rte_kni_info_get is taken by the
> deprecated API), and would work if we add more params to rte_kni_conf.
> 
> Thanks
> marc
> 

Given the issues that have been flagged recently around ABI compatibility, I
don't think I'd introduce such an API. If provided like you describe, it makes
the calling application very dependent upon the size and structure of the
conf structure. Having specific function like this to return specific values
is safer that way.

An alternative is to have a function which returns the conf structure as
a return value, rather than as an in-out arg. That would allow us to add
fields to the end of the structure without breaking applications using that particular
function. This would require us to store the entire conf structure inside the
rte_kni structure though, so we can return a const pointer to it. This is a bit more
invasive of a change.

Overall, I prefer the query-single value option, as I think it's generally the
best-practice for working with structures going forward. [Neil, please correct
me if I'm wrong here!]. If however, you see a current need for looking at the
other fields inside the KNI, I can see about changing things to return the
whole config structure as a const pointer.

/Bruce