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 D0FA0A0613 for ; Tue, 30 Jul 2019 18:50:42 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DD2251BF03; Tue, 30 Jul 2019 18:50:41 +0200 (CEST) Received: from mail-pf1-f196.google.com (mail-pf1-f196.google.com [209.85.210.196]) by dpdk.org (Postfix) with ESMTP id 749BB1BEB2 for ; Tue, 30 Jul 2019 18:50:40 +0200 (CEST) Received: by mail-pf1-f196.google.com with SMTP id p184so30137638pfp.7 for ; Tue, 30 Jul 2019 09:50:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=/lG+ajAaiOl45jfHCWE6/JI928Tr3y6IznaV5RofZDU=; b=lefLcz8Yn2BAGD8Wh4AKkERz7jG1FM9WDX47XUkhpBD8s7FYD9rvRR2WweAnT6PdJA zIeIb47Ah8ACae4s8DjV6jNbLx0fc6in8Pq0mS7bTZUl6CmKcOEmRe2uiqdNzuUyOv1n f25Gw7v/fpjzte8Ct/PaUQKdftPg/enlZMtx3QIMsF5KN5b/LxA010OLlUAWkF1oKDqW eFoeLYNzDmJNHFuEYvmxwjB9EY26x64aR4UA9Ucvq7I7mc7mtBTO0SVoEtnVuZ9dQajl uEMOuR+vsOsbIKFjmtpcz75FIcqVFbp3pwvPdDpZfm85w2Xoda5JGfByGDsUEv3xUqm9 50fQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=/lG+ajAaiOl45jfHCWE6/JI928Tr3y6IznaV5RofZDU=; b=MUwVpqEeZtocLpP9LVEFLymryCI3QxA9c71+YpMuJzATv9twD5aRsByLYFlc3rSNt1 CzMEnHmCjjSS1bwclE6EH+nhjcOxhgwKkkOgSoVdnDG8QqHM/zxi49piqVEmg+PGEA6f RBFGAwVke+NjvMOx+7Gwncg+4mdBcZOCGBfAt17jw7B17rHJ8zI851pEF3n9c5RObVxj KQMscSsiPQQbyrPVFR+UCtxeaBoq9OX1qnv4kbhTs/qBGMuK5CdQIjrKnhE5k5OFIZRS Qov+HmzbtN/Tm6oXMffbABeTyd7Jwl96hW5voCKuPxqPeUgIsk8DHZpaLLSjwG2JzoZY MbeA== X-Gm-Message-State: APjAAAWl75H5tT1oZtESgdyorX6ih88dqeC5p2OCSTUugs5c18NEIaFJ lUwDP0Rm7Zq+NUfCdFN7RS8= X-Google-Smtp-Source: APXvYqzUworLMifI2SNQUK1Ab5kcPRFQpV7SgKeEp7dW4yS1EHIja1dxrKacYm6SN/4qt42isRm6kQ== X-Received: by 2002:a63:d555:: with SMTP id v21mr86909494pgi.179.1564505439346; Tue, 30 Jul 2019 09:50:39 -0700 (PDT) Received: from hermes.lan (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id q7sm72944823pff.2.2019.07.30.09.50.39 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Tue, 30 Jul 2019 09:50:39 -0700 (PDT) Date: Tue, 30 Jul 2019 09:50:32 -0700 From: Stephen Hemminger To: Matan Azrad Cc: "dev@dpdk.org" , Stephen Hemminger Message-ID: <20190730095032.57e1fd05@hermes.lan> In-Reply-To: References: <20190709150939.31338-1-stephen@networkplumber.org> <20190726165054.24078-1-stephen@networkplumber.org> <20190726165054.24078-2-stephen@networkplumber.org> <20190730085656.7c720f28@hermes.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 1/4] examples/multi_process/client_server_mp: check port validity 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" On Tue, 30 Jul 2019 16:39:52 +0000 Matan Azrad wrote: > Hi > > From: Stephen Hemminger > > On Tue, 30 Jul 2019 09:21:02 +0000 > > Matan Azrad wrote: > > > > > Hi Stephen > > > > > > From: Stephen Hemminger > > > > From: Stephen Hemminger > > > > > > > > The mp_server incorrectly allows a port mask that included hidden > > > > ports and which later caused either lost packets or failed initialization. > > > > > > > > This fixes explicitly checking that each bit in portmask is a valid > > > > port before using it. > > > > > > > > Fixes: 5b7ba31148a8 ("ethdev: add port ownership") > > > > Signed-off-by: Stephen Hemminger > > > > --- > > > > .../client_server_mp/mp_server/args.c | 46 +++++++++++++------ > > > > .../client_server_mp/mp_server/args.h | 2 +- > > > > .../client_server_mp/mp_server/init.c | 7 +-- > > > > 3 files changed, 35 insertions(+), 20 deletions(-) > > > > > > > > diff --git > > > > a/examples/multi_process/client_server_mp/mp_server/args.c > > > > b/examples/multi_process/client_server_mp/mp_server/args.c > > > > index b0d8d7665c85..c1ab12ad00d1 100644 > > > > --- a/examples/multi_process/client_server_mp/mp_server/args.c > > > > +++ b/examples/multi_process/client_server_mp/mp_server/args.c > > > > @@ -10,6 +10,7 @@ > > > > #include > > > > > > > > #include > > > > +#include > > > > #include > > > > > > > > #include "common.h" > > > > @@ -34,6 +35,22 @@ usage(void) > > > > , progname); > > > > } > > > > > > > > +/** > > > > + * Check if port is present in the system > > > > + * It maybe owned by a device and should not be used. > > > > + */ > > > > +static int > > > > +port_is_present(uint16_t portid) > > > > +{ > > > > + uint16_t id; > > > > + > > > > + RTE_ETH_FOREACH_DEV(id) { > > > > + if (id == portid) > > > > + return 1; > > > > + } > > > > + return 0; > > > > +} > > > > + > > > > /** > > > > * The ports to be used by the application are passed in > > > > * the form of a bitmask. This function parses the bitmask @@ > > > > -41,31 +58,32 @@ usage(void) > > > > * array variable > > > > */ > > > > static int > > > > -parse_portmask(uint8_t max_ports, const char *portmask) > > > > +parse_portmask(const char *portmask) > > > > { > > > > char *end = NULL; > > > > unsigned long pm; > > > > - uint16_t count = 0; > > > > + uint16_t count; > > > > > > > > if (portmask == NULL || *portmask == '\0') > > > > return -1; > > > > > > > > /* convert parameter to a number and verify */ > > > > pm = strtoul(portmask, &end, 16); > > > > - if (end == NULL || *end != '\0' || pm == 0) > > > > + if (end == NULL || *end != '\0' || pm > UINT16_MAX || pm == 0) > > > > return -1; > > > > > > > > /* loop through bits of the mask and mark ports */ > > > > - while (pm != 0){ > > > > - if (pm & 0x01){ /* bit is set in mask, use port */ > > > > - if (count >= max_ports) > > > > - printf("WARNING: requested port %u not > > > > present" > > > > - " - ignoring\n", (unsigned)count); > > > > - else > > > > - ports->id[ports->num_ports++] = count; > > > > + for (count = 0; pm != 0; pm >>= 1, ++count) { > > > > + if ((pm & 0x1) == 0) > > > > + continue; > > > > + > > > > + if (!port_is_present(count)) { > > > > + printf("WARNING: requested port %u not present - > > > > ignoring\n", > > > > + count); > > > > + continue; > > > > } > > > > - pm = (pm >> 1); > > > > - count++; > > > > + > > > > + ports->id[ports->num_ports++] = count; > > > > } > > > > > > Why not just to do something like: > > > > > > RTE_ETH_FOREACH_DEV(id) { > > > If (pm & (1 << id)) { > > > pm &= ~(1 << id) > > > put in list > > > } > > > } > > > If (pm) > > > Warning > > > > > > > No real reason, was just trying to keep existing logic flow > > This is an example for the users\developers, and you on it, > I think this is a good chance to show how to do it in the correct way - O(n) instead O(n^2). > > Don't you think so? > > I suggest to adjust. > This is a toy example short loops, it doesn't matter. It is important to catch case where portmask contains hidden ports.