| Message ID | 1432734469-1668-1-git-send-email-bruce.richardson@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 3E59F5957; Wed, 27 May 2015 15:47:53 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 314E25946 for <dev@dpdk.org>; Wed, 27 May 2015 15:47:52 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 27 May 2015 06:47:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,505,1427785200"; d="scan'208";a="498914375" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by FMSMGA003.fm.intel.com with ESMTP; 27 May 2015 06:47:49 -0700 Received: from sivswdev01.ir.intel.com (sivswdev01.ir.intel.com [10.237.217.45]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id t4RDlnuU020537; Wed, 27 May 2015 14:47:49 +0100 Received: from sivswdev01.ir.intel.com (localhost [127.0.0.1]) by sivswdev01.ir.intel.com with ESMTP id t4RDlnvu001730; Wed, 27 May 2015 14:47:49 +0100 Received: (from bricha3@localhost) by sivswdev01.ir.intel.com with id t4RDlnOk001722; Wed, 27 May 2015 14:47:49 +0100 From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Date: Wed, 27 May 2015 14:47:49 +0100 Message-Id: <1432734469-1668-1-git-send-email-bruce.richardson@intel.com> X-Mailer: git-send-email 1.7.4.1 Subject: [dpdk-dev] [PATCH 1/4] kni: add function to query the name of a kni object X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Bruce Richardson
May 27, 2015, 1:47 p.m. UTC
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(+)
Comments
On Wed, May 27, 2015 at 02:47:49PM +0100, 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> Apologies on the incorrect subject line. This is a standalone patch, not 1 of 4. The commit just happened to be 4th in my git commit history when I generated the patch and I forgot to update the patch count. :-( /Bruce > --- > 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; > +} > + > /* > * It is deprecated and just for backward compatibility. > */ > diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h > index 44240fe..0c74251 100644 > --- a/lib/librte_kni/rte_kni.h > +++ b/lib/librte_kni/rte_kni.h > @@ -248,6 +248,16 @@ extern uint8_t rte_kni_get_port_id(struct rte_kni *kni) \ > extern struct rte_kni *rte_kni_get(const char *name); > > /** > + * Get the name given to a KNI device > + * > + * @param kni > + * The KNI instance to query > + * @return > + * The pointer to the KNI name > + */ > +extern const char *rte_kni_get_name(const struct rte_kni *kni); > + > +/** > * Get the KNI context of the specific port. > * > * Note: It is deprecated and just for backward compatibility. > diff --git a/lib/librte_kni/rte_kni_version.map b/lib/librte_kni/rte_kni_version.map > index b0bbf4d..e5e4e1b 100644 > --- a/lib/librte_kni/rte_kni_version.map > +++ b/lib/librte_kni/rte_kni_version.map > @@ -6,6 +6,7 @@ DPDK_2.0 { > rte_kni_create; > rte_kni_get; > rte_kni_get_port_id; > + rte_kni_get_name; > rte_kni_handle_request; > rte_kni_info_get; > rte_kni_init; > -- > 2.1.0 >
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 > + > /* > * It is deprecated and just for backward compatibility. > */ > diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h > index 44240fe..0c74251 100644 > --- a/lib/librte_kni/rte_kni.h > +++ b/lib/librte_kni/rte_kni.h > @@ -248,6 +248,16 @@ extern uint8_t rte_kni_get_port_id(struct rte_kni *kni) \ > extern struct rte_kni *rte_kni_get(const char *name); > > /** > + * Get the name given to a KNI device > + * > + * @param kni > + * The KNI instance to query > + * @return > + * The pointer to the KNI name > + */ > +extern const char *rte_kni_get_name(const struct rte_kni *kni); > + > +/** > * Get the KNI context of the specific port. > * > * Note: It is deprecated and just for backward compatibility. > diff --git a/lib/librte_kni/rte_kni_version.map b/lib/librte_kni/rte_kni_version.map > index b0bbf4d..e5e4e1b 100644 > --- a/lib/librte_kni/rte_kni_version.map > +++ b/lib/librte_kni/rte_kni_version.map > @@ -6,6 +6,7 @@ DPDK_2.0 { > rte_kni_create; > rte_kni_get; > rte_kni_get_port_id; > + rte_kni_get_name; > rte_kni_handle_request; > rte_kni_info_get; > rte_kni_init;
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] /Bruce > > >+ > > /* > > * It is deprecated and just for backward compatibility. > > */ > >diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h > >index 44240fe..0c74251 100644 > >--- a/lib/librte_kni/rte_kni.h > >+++ b/lib/librte_kni/rte_kni.h > >@@ -248,6 +248,16 @@ extern uint8_t rte_kni_get_port_id(struct rte_kni *kni) \ > > extern struct rte_kni *rte_kni_get(const char *name); > > /** > >+ * Get the name given to a KNI device > >+ * > >+ * @param kni > >+ * The KNI instance to query > >+ * @return > >+ * The pointer to the KNI name > >+ */ > >+extern const char *rte_kni_get_name(const struct rte_kni *kni); > >+ > >+/** > > * Get the KNI context of the specific port. > > * > > * Note: It is deprecated and just for backward compatibility. > >diff --git a/lib/librte_kni/rte_kni_version.map b/lib/librte_kni/rte_kni_version.map > >index b0bbf4d..e5e4e1b 100644 > >--- a/lib/librte_kni/rte_kni_version.map > >+++ b/lib/librte_kni/rte_kni_version.map > >@@ -6,6 +6,7 @@ DPDK_2.0 { > > rte_kni_create; > > rte_kni_get; > > rte_kni_get_port_id; > >+ rte_kni_get_name; > > rte_kni_handle_request; > > rte_kni_info_get; > > rte_kni_init; >
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 > /Bruce > >>> + >>> /* >>> * It is deprecated and just for backward compatibility. >>> */ >>> diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h >>> index 44240fe..0c74251 100644 >>> --- a/lib/librte_kni/rte_kni.h >>> +++ b/lib/librte_kni/rte_kni.h >>> @@ -248,6 +248,16 @@ extern uint8_t rte_kni_get_port_id(struct rte_kni *kni) \ >>> extern struct rte_kni *rte_kni_get(const char *name); >>> /** >>> + * Get the name given to a KNI device >>> + * >>> + * @param kni >>> + * The KNI instance to query >>> + * @return >>> + * The pointer to the KNI name >>> + */ >>> +extern const char *rte_kni_get_name(const struct rte_kni *kni); >>> + >>> +/** >>> * Get the KNI context of the specific port. >>> * >>> * Note: It is deprecated and just for backward compatibility. >>> diff --git a/lib/librte_kni/rte_kni_version.map b/lib/librte_kni/rte_kni_version.map >>> index b0bbf4d..e5e4e1b 100644 >>> --- a/lib/librte_kni/rte_kni_version.map >>> +++ b/lib/librte_kni/rte_kni_version.map >>> @@ -6,6 +6,7 @@ DPDK_2.0 { >>> rte_kni_create; >>> rte_kni_get; >>> rte_kni_get_port_id; >>> + rte_kni_get_name; >>> rte_kni_handle_request; >>> rte_kni_info_get; >>> rte_kni_init;
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
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson > Sent: Wednesday, May 27, 2015 9:48 PM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH 1/4] kni: add function to query the name of a kni > object > > 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> Acked-by: Helin Zhang <helin.zhang@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; > +} > + > /* > * It is deprecated and just for backward compatibility. > */ > diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h index > 44240fe..0c74251 100644 > --- a/lib/librte_kni/rte_kni.h > +++ b/lib/librte_kni/rte_kni.h > @@ -248,6 +248,16 @@ extern uint8_t rte_kni_get_port_id(struct rte_kni *kni) \ > extern struct rte_kni *rte_kni_get(const char *name); > > /** > + * Get the name given to a KNI device > + * > + * @param kni > + * The KNI instance to query > + * @return > + * The pointer to the KNI name > + */ > +extern const char *rte_kni_get_name(const struct rte_kni *kni); > + > +/** > * Get the KNI context of the specific port. > * > * Note: It is deprecated and just for backward compatibility. > diff --git a/lib/librte_kni/rte_kni_version.map > b/lib/librte_kni/rte_kni_version.map > index b0bbf4d..e5e4e1b 100644 > --- a/lib/librte_kni/rte_kni_version.map > +++ b/lib/librte_kni/rte_kni_version.map > @@ -6,6 +6,7 @@ DPDK_2.0 { > rte_kni_create; > rte_kni_get; > rte_kni_get_port_id; > + rte_kni_get_name; > rte_kni_handle_request; > rte_kni_info_get; > rte_kni_init; > -- > 2.1.0
> > 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> > Acked-by: Helin Zhang <helin.zhang@intel.com> The .map file change was not alphabetical (fixed on the fly). Applied, thanks
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; +} + /* * It is deprecated and just for backward compatibility. */ diff --git a/lib/librte_kni/rte_kni.h b/lib/librte_kni/rte_kni.h index 44240fe..0c74251 100644 --- a/lib/librte_kni/rte_kni.h +++ b/lib/librte_kni/rte_kni.h @@ -248,6 +248,16 @@ extern uint8_t rte_kni_get_port_id(struct rte_kni *kni) \ extern struct rte_kni *rte_kni_get(const char *name); /** + * Get the name given to a KNI device + * + * @param kni + * The KNI instance to query + * @return + * The pointer to the KNI name + */ +extern const char *rte_kni_get_name(const struct rte_kni *kni); + +/** * Get the KNI context of the specific port. * * Note: It is deprecated and just for backward compatibility. diff --git a/lib/librte_kni/rte_kni_version.map b/lib/librte_kni/rte_kni_version.map index b0bbf4d..e5e4e1b 100644 --- a/lib/librte_kni/rte_kni_version.map +++ b/lib/librte_kni/rte_kni_version.map @@ -6,6 +6,7 @@ DPDK_2.0 { rte_kni_create; rte_kni_get; rte_kni_get_port_id; + rte_kni_get_name; rte_kni_handle_request; rte_kni_info_get; rte_kni_init;