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

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

[Bruce Richardson]

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;
+}
+
 /*
  * 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

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

[Bruce Richardson]

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
> 

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

[Marc Sune]

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;

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

[Bruce Richardson]

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;
> 

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

[Bruce Richardson]

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

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

[Zhang, Helin]

> -----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

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

[Thomas Monjalon]

> > 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