Re: [dpdk-dev] [PATCH v5 2/2] app/pdump: enhance to support multi-core capture

[David Marchand <david.marchand@redhat.com>]

On Tue, Apr 2, 2019 at 5:30 PM Varghese, Vipin <vipin.varghese@intel.com>
wrote:

> Hi David,
>
>
>
> snipped
>  #define CMD_LINE_OPT_PDUMP "pdump"
> +#define CMD_LINE_OPT_PDUMP_NUM 1
> +#define CMD_LINE_OPT_MULTI "multi"
> +#define CMD_LINE_OPT_MULTI_NUM 2
>  #define PDUMP_PORT_ARG "port"
>  #define PDUMP_PCI_ARG "device_id"
>  #define PDUMP_QUEUE_ARG "queue"
>
>
>
> You'd better map to integers that do not collide with ascii characters
> (even if non printable).
>
> So values >= 256 are better.
>
> This is the reason for the comment here:
>
> https://git.dpdk.org/dpdk/tree/lib/librte_eal/common/eal_options.h#n19
>
>
>
> In case of dpdk-pdump; there are 2 options ‘multi’ and ‘pdump’. But in
> case of eal_option there are multiple options which has same fist
> character. Hence the comment 'first long only option value must be >= 256,
> so that we won't conflict with short options' is true.
>
>
>
> In my humble opinion, I think it need not change. But please let me know
> otherwise.
>

The convention I had proposed is to just leave the whole [0-255] range to
potential short options.
This marks a demarcation between long options that map to short options and
long "only" options.
I don't care, just prefer to have something systematic.


Snipped
>
> +       printf("usage: %s [EAL options] -- [--%s] "
> +                       "--%s "
>                         "'(port=<port id> | device_id=<pci id or vdev
> name>),"
>                         "(queue=<queue_id>),"
>                         "(rx-dev=<iface or pcap file> |"
> @@ -152,7 +157,7 @@ pdump_usage(const char *prgname)
>                         "[ring-size=<ring size>default:16384],"
>                         "[mbuf-size=<mbuf data size>default:2176],"
>                         "[total-num-mbufs=<number of
> mbufs>default:65535]'\n",
> -                       prgname);
> +                       prgname, CMD_LINE_OPT_MULTI, CMD_LINE_OPT_PDUMP);
>  }
>
>  static int
>
>
>
> You can concatenate the macro.
>
>
>
> Thank you for the suggestion,
>
>
>
> #define OPT(X) #X
>
> #define STR_REPLACE "\n[--"OPT(multi)"]\n --"OPT(pdump)
>
>
> prgname, STR_REPLACE);
>
>
>
> should we change to this or skip this as we are using this once?
>
>
>
> snipped
>

I don't understand what you propose.
My suggestion is simple:
+       printf("usage: %s [EAL options] -- [--"CMD_LINE_OPT_MULTI"] "
+                       "--"CMD_LINE_OPT_PDUMP" "


-- 
David Marchand