Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support.

["Pattan, Reshma" <reshma.pattan@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou
> Sent: Tuesday, April 30, 2019 12:35 PM
> To: dev@dpdk.org
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary
> support.
> 
> When primary app exits, the residual running pdump will stop the primary app to
> restart. Add pdump exits with primary support.
> 
> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>

You need to run below scripts, and fix the issues reported before sending next version.

1)
Ex:
export DPDK_CHECKPATCH_PATH=/usr/src/kernels/4.18.9-200.fc28.x86_64/scripts/checkpatch.pl
[DPDK]# ./devtools/checkpatches.sh

### app/pdump: add pudmp exits with primary support.

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#62: FILE: app/pdump/main.c:559:
+        * function will take care about that. */

total: 0 errors, 1 warnings, 83 lines checked

0/1 valid patch

2)
[DPDK]# ./devtools/check-git-log.sh
Wrong headline format:
        app/pdump: add pudmp exits with primary support.

Need to remove full stop at the end.

Wrong tag:
        Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
        Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>

Might need to remove ,

> @@ -26,6 +26,7 @@
>  #include <rte_ring.h>
>  #include <rte_string_fns.h>
>  #include <rte_pdump.h>
> +#include <rte_alarm.h>
> 

Nitpick:

As per the coding guidelines(https://doc.dpdk.org/guides/contributing/coding_style.html)
the order of the headers is below, so you can  include rte_alarm.h after <rte_eal.h> 

libc includes (system includes first)
DPDK EAL includes
DPDK misc libraries includes
application-specific includes

> +#define MONITOR_INTERVAL (500 * 1000)

Adding comment on how many ms/us would be helpful.

<snipped>
> +	/* Don't worry about it is primary exit case. The alarm cancel
> +	 * function will take care about that. */

Multiline comments should be like below.


/*
 * Multi-line comments look like this.  Make them real sentences. Fill
 * them so they look like real paragraphs.
 */

Source: https://doc.dpdk.org/guides/contributing/coding_style.html


Thanks,
Reshma