diff mbox

[dpdk-dev,v2,1/2] eal: sort and align options lists

Message ID 1422899093-20207-2-git-send-email-thomas.monjalon@6wind.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Thomas Monjalon Feb. 2, 2015, 5:44 p.m. UTC
Options listing in usage help was a mess.
The main usage line is fixed and shorter.
The options in usage output are logically sorted (cpu/mem/dev/proc),
aligned and lightly reworded.
The options in declarations are alphabetically sorted.
Code in swith statement is not moved.

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
changes in v2:
- sort and align options enum in .h
---
 lib/librte_eal/common/eal_common_options.c | 112 ++++++++++++++---------------
 lib/librte_eal/common/eal_options.h        |  62 ++++++++--------
 lib/librte_eal/linuxapp/eal/eal.c          |  19 +++--
 3 files changed, 95 insertions(+), 98 deletions(-)

Comments

David Marchand Feb. 3, 2015, 6:26 a.m. UTC | #1
Two little comments.

On Mon, Feb 2, 2015 at 6:44 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> @@ -578,37 +579,36 @@ eal_check_common_options(struct internal_config
> *internal_cfg)
>  void
>  eal_common_usage(void)
>  {
> -       printf("-c COREMASK -n NUM [-m NB] [-r NUM] [-b
> <domain:bus:devid.func>]"
> -              "[--proc-type primary|secondary|auto]\n\n"
> +       printf("-c COREMASK|-l CORELIST -n CHANNELS [options]\n\n"
>                "EAL common options:\n"
> -              "  -c COREMASK  : A hexadecimal bitmask of cores to run
> on\n"
> -              "  -l CORELIST  : List of cores to run on\n"
> -              "                 The argument format is
> <c1>[-c2][,c3[-c4],...]\n"
>

[snip]


>
> +              "  -n NUM              Number of memory channels\n"
>

Not really a problem, but for consistency : here, you are talking about
NUM, while at first, you wrote -n CHANNELS.


[snip]


>         /* first long only option value must be >= 256, so that we won't
>          * conflict with short options */
>         OPT_LONG_MIN_NUM = 256,
> -#define OPT_HUGE_DIR    "huge-dir"
> -       OPT_HUGE_DIR_NUM = OPT_LONG_MIN_NUM,
> -#define OPT_MASTER_LCORE "master-lcore"
> +#define OPT_BASE_VIRTADDR     "base-virtaddr"
> +       OPT_BASE_VIRTADDR_NUM,
>

Why skip the first entry ?
Afaik, OPT_BASE_VIRTADDR_NUM will be set to 257, is it to avoid having this
= OPT_LONG_MIN_NUM moved anytime we add a new long option at the top of the
enum ?


The rest looks good to me.
Acked-by: David Marchand <david.marchand@6wind.com>
Thomas Monjalon Feb. 3, 2015, 8:27 a.m. UTC | #2
2015-02-03 07:26, David Marchand:
> Two little comments.
> 
> On Mon, Feb 2, 2015 at 6:44 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> > @@ -578,37 +579,36 @@ eal_check_common_options(struct internal_config
> > *internal_cfg)
> >  void
> >  eal_common_usage(void)
> >  {
> > -       printf("-c COREMASK -n NUM [-m NB] [-r NUM] [-b <domain:bus:devid.func>]"
> > -              "[--proc-type primary|secondary|auto]\n\n"
> > +       printf("-c COREMASK|-l CORELIST -n CHANNELS [options]\n\n"
> >                "EAL common options:\n"
> > -              "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
> > -              "  -l CORELIST  : List of cores to run on\n"
> > -              "                 The argument format is <c1>[-c2][,c3[-c4],...]\n"
> [snip]
> > +              "  -n NUM              Number of memory channels\n"
> 
> Not really a problem, but for consistency : here, you are talking about
> NUM, while at first, you wrote -n CHANNELS.

Yes you're right. I changed headline but not the description of this option.
Will do.

> [snip]
> >         /* first long only option value must be >= 256, so that we won't
> >          * conflict with short options */
> >         OPT_LONG_MIN_NUM = 256,
> > -#define OPT_HUGE_DIR    "huge-dir"
> > -       OPT_HUGE_DIR_NUM = OPT_LONG_MIN_NUM,
> > -#define OPT_MASTER_LCORE "master-lcore"
> > +#define OPT_BASE_VIRTADDR     "base-virtaddr"
> > +       OPT_BASE_VIRTADDR_NUM,
> 
> Why skip the first entry ?
> Afaik, OPT_BASE_VIRTADDR_NUM will be set to 257, is it to avoid having this
> = OPT_LONG_MIN_NUM moved anytime we add a new long option at the top of the
> enum ?

Exactly, yes. I think we don't care what is the first number.
It doesn't deserve a painful assignment.

> The rest looks good to me.
> Acked-by: David Marchand <david.marchand@6wind.com>

Thanks
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 67e02dc..4890e78 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -55,37 +55,38 @@ 
 const char
 eal_short_options[] =
 	"b:" /* pci-blacklist */
-	"w:" /* pci-whitelist */
 	"c:" /* coremask */
-	"d:"
+	"d:" /* driver */
 	"l:" /* corelist */
-	"m:"
-	"n:"
-	"r:"
-	"v";
+	"m:" /* memory size */
+	"n:" /* memory channels */
+	"r:" /* memory ranks */
+	"v"  /* version */
+	"w:" /* pci-whitelist */
+	;
 
 const struct option
 eal_long_options[] = {
-	{OPT_HUGE_DIR, 1, 0, OPT_HUGE_DIR_NUM},
-	{OPT_MASTER_LCORE, 1, 0, OPT_MASTER_LCORE_NUM},
-	{OPT_PROC_TYPE, 1, 0, OPT_PROC_TYPE_NUM},
-	{OPT_NO_SHCONF, 0, 0, OPT_NO_SHCONF_NUM},
-	{OPT_NO_HPET, 0, 0, OPT_NO_HPET_NUM},
-	{OPT_VMWARE_TSC_MAP, 0, 0, OPT_VMWARE_TSC_MAP_NUM},
-	{OPT_NO_PCI, 0, 0, OPT_NO_PCI_NUM},
-	{OPT_NO_HUGE, 0, 0, OPT_NO_HUGE_NUM},
-	{OPT_FILE_PREFIX, 1, 0, OPT_FILE_PREFIX_NUM},
-	{OPT_SOCKET_MEM, 1, 0, OPT_SOCKET_MEM_NUM},
-	{OPT_PCI_WHITELIST, 1, 0, OPT_PCI_WHITELIST_NUM},
-	{OPT_PCI_BLACKLIST, 1, 0, OPT_PCI_BLACKLIST_NUM},
-	{OPT_VDEV, 1, 0, OPT_VDEV_NUM},
-	{OPT_SYSLOG, 1, NULL, OPT_SYSLOG_NUM},
-	{OPT_LOG_LEVEL, 1, NULL, OPT_LOG_LEVEL_NUM},
-	{OPT_BASE_VIRTADDR, 1, 0, OPT_BASE_VIRTADDR_NUM},
-	{OPT_XEN_DOM0, 0, 0, OPT_XEN_DOM0_NUM},
-	{OPT_CREATE_UIO_DEV, 1, NULL, OPT_CREATE_UIO_DEV_NUM},
-	{OPT_VFIO_INTR, 1, NULL, OPT_VFIO_INTR_NUM},
-	{0, 0, 0, 0}
+	{OPT_BASE_VIRTADDR,     1, NULL, OPT_BASE_VIRTADDR_NUM    },
+	{OPT_CREATE_UIO_DEV,    1, NULL, OPT_CREATE_UIO_DEV_NUM   },
+	{OPT_FILE_PREFIX,       1, NULL, OPT_FILE_PREFIX_NUM      },
+	{OPT_HUGE_DIR,          1, NULL, OPT_HUGE_DIR_NUM         },
+	{OPT_LOG_LEVEL,         1, NULL, OPT_LOG_LEVEL_NUM        },
+	{OPT_MASTER_LCORE,      1, NULL, OPT_MASTER_LCORE_NUM     },
+	{OPT_NO_HPET,           0, NULL, OPT_NO_HPET_NUM          },
+	{OPT_NO_HUGE,           0, NULL, OPT_NO_HUGE_NUM          },
+	{OPT_NO_PCI,            0, NULL, OPT_NO_PCI_NUM           },
+	{OPT_NO_SHCONF,         0, NULL, OPT_NO_SHCONF_NUM        },
+	{OPT_PCI_BLACKLIST,     1, NULL, OPT_PCI_BLACKLIST_NUM    },
+	{OPT_PCI_WHITELIST,     1, NULL, OPT_PCI_WHITELIST_NUM    },
+	{OPT_PROC_TYPE,         1, NULL, OPT_PROC_TYPE_NUM        },
+	{OPT_SOCKET_MEM,        1, NULL, OPT_SOCKET_MEM_NUM       },
+	{OPT_SYSLOG,            1, NULL, OPT_SYSLOG_NUM           },
+	{OPT_VDEV,              1, NULL, OPT_VDEV_NUM             },
+	{OPT_VFIO_INTR,         1, NULL, OPT_VFIO_INTR_NUM        },
+	{OPT_VMWARE_TSC_MAP,    0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
+	{OPT_XEN_DOM0,          0, NULL, OPT_XEN_DOM0_NUM         },
+	{0,                     0, NULL, 0                        }
 };
 
 static int lcores_parsed;
@@ -578,37 +579,36 @@  eal_check_common_options(struct internal_config *internal_cfg)
 void
 eal_common_usage(void)
 {
-	printf("-c COREMASK -n NUM [-m NB] [-r NUM] [-b <domain:bus:devid.func>]"
-	       "[--proc-type primary|secondary|auto]\n\n"
+	printf("-c COREMASK|-l CORELIST -n CHANNELS [options]\n\n"
 	       "EAL common options:\n"
-	       "  -c COREMASK  : A hexadecimal bitmask of cores to run on\n"
-	       "  -l CORELIST  : List of cores to run on\n"
-	       "                 The argument format is <c1>[-c2][,c3[-c4],...]\n"
-	       "                 where c1, c2, etc are core indexes between 0 and %d\n"
-	       "  --"OPT_MASTER_LCORE" ID: Core ID that is used as master\n"
-	       "  -n NUM       : Number of memory channels\n"
-	       "  -v           : Display version information on startup\n"
-	       "  -m MB        : memory to allocate (see also --"OPT_SOCKET_MEM")\n"
-	       "  -r NUM       : force number of memory ranks (don't detect)\n"
-	       "  --"OPT_SYSLOG"     : set syslog facility\n"
-	       "  --"OPT_LOG_LEVEL"  : set default log level\n"
-	       "  --"OPT_PROC_TYPE"  : type of this process\n"
-	       "  --"OPT_PCI_BLACKLIST", -b: add a PCI device in black list.\n"
-	       "               Prevent EAL from using this PCI device. The argument\n"
-	       "               format is <domain:bus:devid.func>.\n"
-	       "  --"OPT_PCI_WHITELIST", -w: add a PCI device in white list.\n"
-	       "               Only use the specified PCI devices. The argument format\n"
-	       "               is <[domain:]bus:devid.func>. This option can be present\n"
-	       "               several times (once per device).\n"
-	       "               [NOTE: PCI whitelist cannot be used with -b option]\n"
-	       "  --"OPT_VDEV": add a virtual device.\n"
-	       "               The argument format is <driver><id>[,key=val,...]\n"
-	       "               (ex: --vdev=eth_pcap0,iface=eth2).\n"
-	       "  --"OPT_VMWARE_TSC_MAP": use VMware TSC map instead of native RDTSC\n"
+	       "  -c COREMASK         Hexadecimal bitmask of cores to run on\n"
+	       "  -l CORELIST         List of cores to run on\n"
+	       "                      The argument format is <c1>[-c2][,c3[-c4],...]\n"
+	       "                      where c1, c2, etc are core indexes between 0 and %d\n"
+	       "  --"OPT_MASTER_LCORE" ID   Core ID that is used as master\n"
+	       "  -n NUM              Number of memory channels\n"
+	       "  -m MB               Memory to allocate (see also --"OPT_SOCKET_MEM")\n"
+	       "  -r NUM              Force number of memory ranks (don't detect)\n"
+	       "  -b, --"OPT_PCI_BLACKLIST" Add a PCI device in black list.\n"
+	       "                      Prevent EAL from using this PCI device. The argument\n"
+	       "                      format is <domain:bus:devid.func>.\n"
+	       "  -w, --"OPT_PCI_WHITELIST" Add a PCI device in white list.\n"
+	       "                      Only use the specified PCI devices. The argument format\n"
+	       "                      is <[domain:]bus:devid.func>. This option can be present\n"
+	       "                      several times (once per device).\n"
+	       "                      [NOTE: PCI whitelist cannot be used with -b option]\n"
+	       "  --"OPT_VDEV"              Add a virtual device.\n"
+	       "                      The argument format is <driver><id>[,key=val,...]\n"
+	       "                      (ex: --vdev=eth_pcap0,iface=eth2).\n"
+	       "  --"OPT_VMWARE_TSC_MAP"    Use VMware TSC map instead of native RDTSC\n"
+	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
+	       "  --"OPT_SYSLOG"            Set syslog facility\n"
+	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
+	       "  -v                  Display version information on startup\n"
 	       "\nEAL options for DEBUG use only:\n"
-	       "  --"OPT_NO_HUGE"  : use malloc instead of hugetlbfs\n"
-	       "  --"OPT_NO_PCI"   : disable pci\n"
-	       "  --"OPT_NO_HPET"  : disable hpet\n"
-	       "  --"OPT_NO_SHCONF": no shared config (mmap'd files)\n"
+	       "  --"OPT_NO_HUGE"           Use malloc instead of hugetlbfs\n"
+	       "  --"OPT_NO_PCI"            Disable PCI\n"
+	       "  --"OPT_NO_HPET"           Disable HPET\n"
+	       "  --"OPT_NO_SHCONF"         No shared config (mmap'd files)\n"
 	       "\n", RTE_MAX_LCORE);
 }
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index e476f8d..fe1c85d 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -35,48 +35,48 @@ 
 
 enum {
 	/* long options mapped to a short option */
-#define OPT_PCI_WHITELIST "pci-whitelist"
-	OPT_PCI_WHITELIST_NUM = 'w',
-#define OPT_PCI_BLACKLIST "pci-blacklist"
-	OPT_PCI_BLACKLIST_NUM = 'b',
+#define OPT_PCI_BLACKLIST     "pci-blacklist"
+	OPT_PCI_BLACKLIST_NUM   = 'b',
+#define OPT_PCI_WHITELIST     "pci-whitelist"
+	OPT_PCI_WHITELIST_NUM   = 'w',
 
 	/* first long only option value must be >= 256, so that we won't
 	 * conflict with short options */
 	OPT_LONG_MIN_NUM = 256,
-#define OPT_HUGE_DIR    "huge-dir"
-	OPT_HUGE_DIR_NUM = OPT_LONG_MIN_NUM,
-#define OPT_MASTER_LCORE "master-lcore"
+#define OPT_BASE_VIRTADDR     "base-virtaddr"
+	OPT_BASE_VIRTADDR_NUM,
+#define OPT_CREATE_UIO_DEV    "create-uio-dev"
+	OPT_CREATE_UIO_DEV_NUM,
+#define OPT_FILE_PREFIX       "file-prefix"
+	OPT_FILE_PREFIX_NUM,
+#define OPT_HUGE_DIR          "huge-dir"
+	OPT_HUGE_DIR_NUM,
+#define OPT_LOG_LEVEL         "log-level"
+	OPT_LOG_LEVEL_NUM,
+#define OPT_MASTER_LCORE      "master-lcore"
 	OPT_MASTER_LCORE_NUM,
-#define OPT_PROC_TYPE   "proc-type"
+#define OPT_PROC_TYPE         "proc-type"
 	OPT_PROC_TYPE_NUM,
-#define OPT_NO_SHCONF   "no-shconf"
-	OPT_NO_SHCONF_NUM,
-#define OPT_NO_HPET     "no-hpet"
+#define OPT_NO_HPET           "no-hpet"
 	OPT_NO_HPET_NUM,
-#define OPT_VMWARE_TSC_MAP   "vmware-tsc-map"
-	OPT_VMWARE_TSC_MAP_NUM,
-#define OPT_NO_PCI      "no-pci"
-	OPT_NO_PCI_NUM,
-#define OPT_NO_HUGE     "no-huge"
+#define OPT_NO_HUGE           "no-huge"
 	OPT_NO_HUGE_NUM,
-#define OPT_FILE_PREFIX "file-prefix"
-	OPT_FILE_PREFIX_NUM,
-#define OPT_SOCKET_MEM  "socket-mem"
+#define OPT_NO_PCI            "no-pci"
+	OPT_NO_PCI_NUM,
+#define OPT_NO_SHCONF         "no-shconf"
+	OPT_NO_SHCONF_NUM,
+#define OPT_SOCKET_MEM        "socket-mem"
 	OPT_SOCKET_MEM_NUM,
-#define OPT_VDEV        "vdev"
-	OPT_VDEV_NUM,
-#define OPT_SYSLOG      "syslog"
+#define OPT_SYSLOG            "syslog"
 	OPT_SYSLOG_NUM,
-#define OPT_LOG_LEVEL   "log-level"
-	OPT_LOG_LEVEL_NUM,
-#define OPT_BASE_VIRTADDR   "base-virtaddr"
-	OPT_BASE_VIRTADDR_NUM,
-#define OPT_XEN_DOM0    "xen-dom0"
-	OPT_XEN_DOM0_NUM,
-#define OPT_CREATE_UIO_DEV "create-uio-dev"
-	OPT_CREATE_UIO_DEV_NUM,
-#define OPT_VFIO_INTR    "vfio-intr"
+#define OPT_VDEV              "vdev"
+	OPT_VDEV_NUM,
+#define OPT_VFIO_INTR         "vfio-intr"
 	OPT_VFIO_INTR_NUM,
+#define OPT_VMWARE_TSC_MAP    "vmware-tsc-map"
+	OPT_VMWARE_TSC_MAP_NUM,
+#define OPT_XEN_DOM0          "xen-dom0"
+	OPT_XEN_DOM0_NUM,
 	OPT_LONG_MAX_NUM
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index f99e158..e3955e7 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -352,17 +352,14 @@  eal_usage(const char *prgname)
 	printf("\nUsage: %s ", prgname);
 	eal_common_usage();
 	printf("EAL Linux options:\n"
-	       "  -d LIB.so    : add driver (can be used multiple times)\n"
-	       "  --"OPT_XEN_DOM0" : support application running on Xen Domain0 "
-			   "without hugetlbfs\n"
-	       "  --"OPT_SOCKET_MEM" : memory to allocate on specific\n"
-		   "                 sockets (use comma separated values)\n"
-	       "  --"OPT_HUGE_DIR"   : directory where hugetlbfs is mounted\n"
-	       "  --"OPT_FILE_PREFIX": prefix for hugepage filenames\n"
-	       "  --"OPT_BASE_VIRTADDR": specify base virtual address\n"
-	       "  --"OPT_VFIO_INTR": specify desired interrupt mode for VFIO "
-			   "(legacy|msi|msix)\n"
-	       "  --"OPT_CREATE_UIO_DEV": create /dev/uioX (usually done by hotplug)\n"
+	       "  -d LIB.so           Add driver (can be used multiple times)\n"
+	       "  --"OPT_SOCKET_MEM"        Memory to allocate on sockets (comma separated values)\n"
+	       "  --"OPT_HUGE_DIR"          Directory where hugetlbfs is mounted\n"
+	       "  --"OPT_FILE_PREFIX"       Prefix for hugepage filenames\n"
+	       "  --"OPT_BASE_VIRTADDR"     Base virtual address\n"
+	       "  --"OPT_CREATE_UIO_DEV"    Create /dev/uioX (usually done by hotplug)\n"
+	       "  --"OPT_VFIO_INTR"         Interrupt mode for VFIO (legacy|msi|msix)\n"
+	       "  --"OPT_XEN_DOM0"          Support running on Xen dom0 without hugetlbfs\n"
 	       "\n");
 	/* Allow the application to print its usage message too if hook is set */
 	if ( rte_application_usage_hook ) {