From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 9D241A04DC; Tue, 20 Oct 2020 14:43:29 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 469D0A976; Tue, 20 Oct 2020 14:43:28 +0200 (CEST) Received: from wout5-smtp.messagingengine.com (wout5-smtp.messagingengine.com [64.147.123.21]) by dpdk.org (Postfix) with ESMTP id 69A64A96E for ; Tue, 20 Oct 2020 14:43:25 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.west.internal (Postfix) with ESMTP id 4237B252; Tue, 20 Oct 2020 08:43:23 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Tue, 20 Oct 2020 08:43:23 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm2; bh= dCAdUmujY8Tv+PisbPoPQ3Fw0AI5IVz6SZjXhefCLfU=; b=LiUjkuYowymDlf0E 6FICX6ITQvi2x4lnjEVPDxO62vBpDX5ESaW3SXCoUzNHK4AMVu/5g1duGcGYmjtf YmSEpBcZB3E7F+P4zAbyn1rhOafSybWTI40NlST9Ew1YgB35av5rKZpbD6ibUvJH jqX5J8YjWYyz89v7LY/nkRaC3bcuhDW7WuJiC70bOVcgfyFcEsCbW7f4j23YYpu7 nKVz+d2UhzdArsn+TnATN7BW48icbkcKEkGO03gvGFoOGl4MP1mcz4To4x2RcSGo h05WY5Bph8n1LVWByL5MreuNAUvnS7UK1sRNk18oMA7JPZ5I9XPqyMIi9GpNU5HP crbDzA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=dCAdUmujY8Tv+PisbPoPQ3Fw0AI5IVz6SZjXhefCL fU=; b=Hi1OqzLxGNBMZQ8y5+n8ftLsneMzg1E99GX4LVCf88RASNK0izTox+/gx B6RWVOs2c6SXKyABx5EccRfmLeh+aZg/MZ++LRoeeAi6Ou+ff+/yb2qj6SsMETVG fi6FKP/GSrckJA5Yw4IeWNUHo4DvM2JkNR6WDvM7ijiYYV2OWtq/ECPtoDggSSbD srVWw6doFT55cXE2xDt/lIrIXzAl+s6qhwMFVvQmIkZG2BkV5eSg1oBkd/h4LB0M 8WY7KcFEppKjEheuTicXMu3WrgYWByVkwiGJ4PpFFC1+rc3RiFbndDdYj2IixCD6 CN2T3mOCyW2lgSN73pIIgmzid5nSA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrjeefgdehgecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpeefgfehveevfedtfffgkedvhfegtdefkedtkefgvdffjeektefhleej tdegleelffenucffohhmrghinhepughpughkrdhorhhgpdhfuhhntgdrnhdqrgenucfkph epjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghr rghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id D3ACC3280063; Tue, 20 Oct 2020 08:43:21 -0400 (EDT) From: Thomas Monjalon To: Stephen Hemminger Cc: dev@dpdk.org, Luca Boccassi , David Marchand Date: Tue, 20 Oct 2020 14:43:20 +0200 Message-ID: <9162316.KZ3Jag88Df@thomas> In-Reply-To: References: <20200922143202.8755-1-stephen@networkplumber.org> <20200922143202.8755-5-stephen@networkplumber.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH 4/8] eal: replace pci-whitelist/pci-blacklist options X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Ping Stephen, Waiting a new version addressing these comments before resuming the review. Thanks 14/10/2020 18:07, David Marchand: > On Tue, Sep 22, 2020 at 4:33 PM Stephen Hemminger > wrote: > > > > Replace -w / --pci-whitelist with -a / --allow options > > and -b / --pci-blacklist with -b / --block. > > Looks like there is a mix or -a/-b and -i/-x from the previous proposal. > > This is what causes CI failure as I previously reported: > http://inbox.dpdk.org/dev/CAJFAV8yCWg9YXKBiKsoEq7dHGr3PjbTGzHL_d1j5+Dp9GuXbOw@mail.gmail.com/ > > I guess the intention of this patch is: > > '-b' / '--pci-blacklist' becomes '-b' / '--block' + warning on '--pci-blacklist' > '-w' / '--pci-whitelist' becomes '-a' / '--allow' + warning on 'w' / > '--pci-whitelist' > > > Comments below. > > > > > > Allow the old options for now, but print a nag > > warning since old options are deprecated. > > > > Signed-off-by: Stephen Hemminger > > Acked-by: Luca Boccassi > > --- > > lib/librte_eal/common/eal_common_options.c | 70 +++++++++++++--------- > > lib/librte_eal/common/eal_options.h | 9 ++- > > 2 files changed, 51 insertions(+), 28 deletions(-) > > > > diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c > > index a5426e12346a..4e54512c874c 100644 > > --- a/lib/librte_eal/common/eal_common_options.c > > +++ b/lib/librte_eal/common/eal_common_options.c > > @@ -51,7 +51,8 @@ > > > > const char > > eal_short_options[] = > > - "b:" /* pci-blacklist */ > > + "a:" /* allow */ > > + "b:" /* block */ > > "c:" /* coremask */ > > "s:" /* service coremask */ > > "d:" /* driver */ > > @@ -62,7 +63,7 @@ eal_short_options[] = > > "n:" /* memory channels */ > > "r:" /* memory ranks */ > > "v" /* version */ > > - "w:" /* pci-whitelist */ > > + "w:" /* whitelist (deprecated) */ > > ; > > > > const struct option > > @@ -87,8 +88,8 @@ eal_long_options[] = { > > {OPT_NO_PCI, 0, NULL, OPT_NO_PCI_NUM }, > > {OPT_NO_SHCONF, 0, NULL, OPT_NO_SHCONF_NUM }, > > {OPT_IN_MEMORY, 0, NULL, OPT_IN_MEMORY_NUM }, > > - {OPT_PCI_BLACKLIST, 1, NULL, OPT_PCI_BLACKLIST_NUM }, > > - {OPT_PCI_WHITELIST, 1, NULL, OPT_PCI_WHITELIST_NUM }, > > + {OPT_DEV_BLOCK, 1, NULL, OPT_DEV_BLOCK_NUM }, > > + {OPT_DEV_ALLOW, 1, NULL, OPT_DEV_ALLOW_NUM }, > > {OPT_PROC_TYPE, 1, NULL, OPT_PROC_TYPE_NUM }, > > {OPT_SOCKET_MEM, 1, NULL, OPT_SOCKET_MEM_NUM }, > > {OPT_SOCKET_LIMIT, 1, NULL, OPT_SOCKET_LIMIT_NUM }, > > @@ -102,6 +103,11 @@ eal_long_options[] = { > > {OPT_MATCH_ALLOCATIONS, 0, NULL, OPT_MATCH_ALLOCATIONS_NUM}, > > {OPT_TELEMETRY, 0, NULL, OPT_TELEMETRY_NUM }, > > {OPT_NO_TELEMETRY, 0, NULL, OPT_NO_TELEMETRY_NUM }, > > + > > + /* legacy options that will be removed in next LTS */ > > + {OPT_PCI_BLACKLIST, 1, NULL, OPT_PCI_BLACKLIST_NUM }, > > + {OPT_PCI_WHITELIST, 1, NULL, OPT_PCI_WHITELIST_NUM }, > > + > > {0, 0, NULL, 0 } > > }; > > > > @@ -1414,29 +1420,38 @@ int > > eal_parse_common_option(int opt, const char *optarg, > > struct internal_config *conf) > > { > > - static int b_used; > > - static int w_used; > > + static bool x_used, i_used; > > Previously the variables matched the short options. > It should be a_used and b_used. > > > > > > switch (opt) { > > - /* blacklist */ > > + /* deprecated option */ > > case 'b': > > Should be > case OPT_PCI_BLACKLIST_NUM: > > > - if (w_used) > > - goto bw_used; > > - if (eal_option_device_add(RTE_DEVTYPE_BLACKLISTED_PCI, > > + fprintf(stderr, > > + "Option -b, --blacklist is deprecated, use -x, --exclude option instead\n"); > > We keep the -b option as is, don't complain about it. > > Plus, the previous option was not --blacklist but --pci-blacklist. > Please use "--"OPT_PCI_BLACKLIST. > > > > > + /* fallthrough */ > > + case 'x': > > + /* excluded list */ > > Should be > case 'b': > /* block list */ > > > + if (i_used) > > + goto include_exclude; > > + if (eal_option_device_add(RTE_DEVTYPE_BLOCKED, > > optarg) < 0) { > > return -1; > > } > > - b_used = 1; > > + x_used = true; > > break; > > - /* whitelist */ > > + > > case 'w': > > - if (b_used) > > - goto bw_used; > > - if (eal_option_device_add(RTE_DEVTYPE_WHITELISTED_PCI, > > + fprintf(stderr, > > + "Option -w, --whitelist is deprecated, use -i, --include option instead\n"); > > Plus, the previous option was not --whitelist but --pci-whitelist. > Please use "--"OPT_PCI_WHITELIST. > > The new option is '-a' not '-i'. > > > > > + /* fallthrough */ > > + case 'i': > > + /* include device list */ > > Should be > case 'a': > /* allow list */ > > etc... > > > > + if (x_used) > > + goto include_exclude; > > + if (eal_option_device_add(RTE_DEVTYPE_ALLOWED, > > optarg) < 0) { > > return -1; > > } > > - w_used = 1; > > + i_used = true; > > break; > > /* coremask */ > > case 'c': { > > @@ -1715,9 +1730,10 @@ eal_parse_common_option(int opt, const char *optarg, > > } > > > > return 0; > > -bw_used: > > - RTE_LOG(ERR, EAL, "Options blacklist (-b) and whitelist (-w) " > > - "cannot be used at the same time\n"); > > + > > +include_exclude: > > + RTE_LOG(ERR, EAL, > > + "Options include (-i) and exclude (-x) can't be used at the same time\n"); > > -a / -b > > > > return -1; > > } > > > > @@ -1926,14 +1942,14 @@ eal_common_usage(void) > > " -n CHANNELS Number of memory channels\n" > > " -m MB Memory to allocate (see also --"OPT_SOCKET_MEM")\n" > > " -r RANKS 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 .\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" > > + " -b, --"OPT_DEV_BLOCK" Add a device to the excluded list.\n" > > + " Prevent EAL from using this device. The argument\n" > > + " format for PCI devices is .\n" > > + " -a, --"OPT_DEV_ALLOW" Add a device to the included list.\n" > > + " Only use the specified devices. The argument format\n" > > + " for PCI devices is <[domain:]bus:devid.func>.\n" > > + " This option can be present several times.\n" > > + " [NOTE: " OPT_DEV_ALLOW " cannot be used with "OPT_DEV_BLOCK" option]\n" > > " --"OPT_VDEV" Add a virtual device.\n" > > " The argument format is [,key=val,...]\n" > > " (ex: --vdev=net_pcap0,iface=eth2).\n" > > diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h > > index 89769d48b487..fc0fe2258a32 100644 > > --- a/lib/librte_eal/common/eal_options.h > > +++ b/lib/librte_eal/common/eal_options.h > > @@ -13,8 +13,15 @@ enum { > > /* long options mapped to a short option */ > > #define OPT_HELP "help" > > OPT_HELP_NUM = 'h', > > +#define OPT_DEV_ALLOW "allow" > > + OPT_DEV_ALLOW_NUM = 'a', > > +#define OPT_DEV_BLOCK "block" > > + OPT_DEV_BLOCK_NUM = 'b', > > Fix indent. > > > > + > > + /* legacy options that will be removed in next LTS */ > > #define OPT_PCI_BLACKLIST "pci-blacklist" > > - OPT_PCI_BLACKLIST_NUM = 'b', > > +#define OPT_PCI_BLACKLIST_NUM OPT_DEV_BLOCK_NUM > > Move OPT_PCI_BLACKLIST_NUM as a long-only option. > This way we can differentiate it from the 'b' short option that has > been preserved. > > > > + > > #define OPT_PCI_WHITELIST "pci-whitelist" > > OPT_PCI_WHITELIST_NUM = 'w', > > > > -- > > 2.27.0 > > > > >