Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

2018-04-05 Thread Tan, Jianfeng 

Sorry, it seems that I missed this email.


On 4/5/2018 5:45 PM, Pattan, Reshma wrote:

Hi


Signed-off-by: Jianfeng Tan 
---
Note this patch needs this patch set:
http://dpdk.org/dev/patchwork/patch/36814/
v2:
   - Update doc for deprecation of API, rte_pdump_set_socket_dir,
 and API change for rte_pdump_init.
   - Add notice for known incompatibility issue in doc.
  app/pdump/main.c   |   6 +-
  doc/guides/rel_notes/deprecation.rst   |   4 +
  doc/guides/rel_notes/release_18_05.rst |   7 +
  lib/librte_pdump/Makefile  |   3 +-
  lib/librte_pdump/rte_pdump.c   | 423 +
  lib/librte_pdump/rte_pdump.h   |   1 +
  6 files changed, 84 insertions(+), 360 deletions(-)
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
+




+
+   /* recv client requests */
+   if (mp_msg->len_param != sizeof(*cli_req)) {
+   RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
+   resp->err_value = EINVAL;

resp->err_value = -EINVAL


OK, this keeps consistent with other places in this file.




-   /* save the socket in local configuration */
-   pdump_socket_fd = socket_fd;
+   snprintf(mp_resp.name, RTE_MP_MAX_NAME_LEN, PDUMP_MP);
+   mp_resp.len_param = sizeof(*resp);
+   mp_resp.num_fds = 0;
+   if (rte_mp_reply(&mp_resp, peer) < 0)
+   RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
+   strerror(rte_errno), __func__, __LINE__);


If failed to send the reply should'nt we return -1?


Yes, will fix it.




return 0;
  }

  int
-rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype
type)
+rte_pdump_set_socket_dir(const char *path __rte_unused,
+enum rte_pdump_socktype type __rte_unused)
  {

What about enum rte_pdump_socktype in header file? When to delete them?


Will deprecate it.



We need to update doxygen comments in header file for rte_pdump_init and 
rte_pdump_uninit()? What do you say.


I'll try to find a flag for deprecating a param. If you know how to do 
that, please let me know.


Thanks,
Jianfeng



Thanks,
Reshma

Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

2018-04-05 Thread Tan, Jianfeng 



On 4/5/2018 6:37 PM, Pattan, Reshma wrote:

Hi,


-Original Message-
From: Tan, Jianfeng
Sent: Wednesday, April 4, 2018 4:08 PM
To: dev@dpdk.org
Cc: Tan, Jianfeng ; Pattan, Reshma

Subject: [PATCH v2] pdump: change to use generic multi-process channel

The original code replies on the private channel for primary and secondary
communication. Change to use the generic multi-process channel.

Note with this change, dpdk-pdump will be not compatible with old version
DPDK applications.

Cc: reshma.pat...@intel.com

Signed-off-by: Jianfeng Tan 
---
diff --git a/doc/guides/rel_notes/deprecation.rst
b/doc/guides/rel_notes/deprecation.rst
index 0c696f7..d55fd05 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -153,3 +153,7 @@ Deprecation Notices
be added between the producer and consumer structures. The size of the
structure and the offset of the fields will remain the same on
platforms with 64B cache line, but will change on other platforms.
+
+* pdump: As we changed to use generic IPC, ``rte_pdump_set_socket_dir``
+will be
+  deprecated and removed in subsequent release; the parameter, path, of
+  ``rte_pdump_init`` will also be removed.

Do we need to mention about deprecation of below enums too?
enum rte_pdump_socktype {
 RTE_PDUMP_SOCKET_SERVER = 1,
 RTE_PDUMP_SOCKET_CLIENT = 2
};


Nice catch, will add in next version.

Thanks,
Jianfeng



Thanks,
Reshma

Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

2018-04-05 Thread Pattan, Reshma 
Hi,

> -Original Message-
> From: Tan, Jianfeng
> Sent: Wednesday, April 4, 2018 4:08 PM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng ; Pattan, Reshma
> 
> Subject: [PATCH v2] pdump: change to use generic multi-process channel
> 
> The original code replies on the private channel for primary and secondary
> communication. Change to use the generic multi-process channel.
> 
> Note with this change, dpdk-pdump will be not compatible with old version
> DPDK applications.
> 
> Cc: reshma.pat...@intel.com
> 
> Signed-off-by: Jianfeng Tan 
> ---
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 0c696f7..d55fd05 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -153,3 +153,7 @@ Deprecation Notices
>be added between the producer and consumer structures. The size of the
>structure and the offset of the fields will remain the same on
>platforms with 64B cache line, but will change on other platforms.
> +
> +* pdump: As we changed to use generic IPC, ``rte_pdump_set_socket_dir``
> +will be
> +  deprecated and removed in subsequent release; the parameter, path, of
> +  ``rte_pdump_init`` will also be removed.

Do we need to mention about deprecation of below enums too?
enum rte_pdump_socktype {
RTE_PDUMP_SOCKET_SERVER = 1,
RTE_PDUMP_SOCKET_CLIENT = 2
};

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

2018-04-05 Thread Pattan, Reshma 
Hi

> 
> Signed-off-by: Jianfeng Tan 
> ---
> Note this patch needs this patch set:
> http://dpdk.org/dev/patchwork/patch/36814/
> v2:
>   - Update doc for deprecation of API, rte_pdump_set_socket_dir,
> and API change for rte_pdump_init.
>   - Add notice for known incompatibility issue in doc.
>  app/pdump/main.c   |   6 +-
>  doc/guides/rel_notes/deprecation.rst   |   4 +
>  doc/guides/rel_notes/release_18_05.rst |   7 +
>  lib/librte_pdump/Makefile  |   3 +-
>  lib/librte_pdump/rte_pdump.c   | 423 
> +
>  lib/librte_pdump/rte_pdump.h   |   1 +
>  6 files changed, 84 insertions(+), 360 deletions(-)

>diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
> +
> 


> +
> + /* recv client requests */
> + if (mp_msg->len_param != sizeof(*cli_req)) {
> + RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
> + resp->err_value = EINVAL;

resp->err_value = -EINVAL

> - /* save the socket in local configuration */
> - pdump_socket_fd = socket_fd;
> + snprintf(mp_resp.name, RTE_MP_MAX_NAME_LEN, PDUMP_MP);
> + mp_resp.len_param = sizeof(*resp);
> + mp_resp.num_fds = 0;
> + if (rte_mp_reply(&mp_resp, peer) < 0)
> + RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
> + strerror(rte_errno), __func__, __LINE__);
> 

If failed to send the reply should'nt we return -1? 

>   return 0;
>  }
> 

>  int
> -rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype
> type)
> +rte_pdump_set_socket_dir(const char *path __rte_unused,
> +  enum rte_pdump_socktype type __rte_unused)
>  {

What about enum rte_pdump_socktype in header file? When to delete them?

We need to update doxygen comments in header file for rte_pdump_init and 
rte_pdump_uninit()? What do you say.

Thanks,
Reshma