From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 3EE2F1B016 for ; Tue, 16 Jan 2018 11:54:57 +0100 (CET) Received: by mail-wm0-f66.google.com with SMTP id 143so7381159wma.5 for ; Tue, 16 Jan 2018 02:54:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=rYdtt4r2ZeKeXI9VxQ2oTYjSzFV2tbk14OR95WK17vw=; b=clAuvSmhGAwUi1rXMbGmsGpo9tiFgMJWdKJlNwOV/OgyhT8CcUQ5KiIS82JsH3fktK eZFXwFglU9qgQS5wQJ/7+YvuwJj1pD9jIHyYCj3CFLXnVgG2ocL/Nq4F5hBa6KBLaU5+ OdGhzJjExwkSiiZ8cTEHpvgFprWUJd9UrM1f7kCwb7eOdBapA68LGnP+3A2PK+kzfOCU 6x3xcqMMBaakWwxZY+qN+OsUK1B1uqtbAs1uMrccLACF4CNQZD9Ygv+DeB6wSRJPziFH yB46MgmD5lu3OwvnQWIg+ltecG3a8QmcU64bSS4k/+zaUkVSXKyU21aguCFySBZPwXdT FNMg== 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:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=rYdtt4r2ZeKeXI9VxQ2oTYjSzFV2tbk14OR95WK17vw=; b=Szdj/SL4RPsDFzdqQTFDwJU6Kpk1qamapjqyuYEFaQgWED7d+sLq271RlWY5K49/mo vlO7pLxsv8hWCLL8eB9b6Dlm/CMlLe9FSQ1EhFkmvGw3xN7dHR3X3uFNtygQrark/EUZ Tqv657zzQrsfVzYIXpgtyU+l9KzklLSsq8BCLlfr8GSu15/nBTCnQ01bGsMbkQIiQsTU Sxejb7JXdbjJvlTUiv9xjorBqQ2uveCiccOwfFB9+fFrQQhTe7u8D1OySL3ftMKthUc9 al803jgx9r7V8upU66l9xDYi3R8CuoP/ua/DMRTqrtrXPZrPHP1g40ycDJMFZQEbB110 ovTQ== X-Gm-Message-State: AKwxytdPEY/U2yXeGA3U2l3jYx+HRg6ou28j6414T2wtuUsmF8P8sx7G yVUqgimxfe2BN7XiOGQQdvUryw== X-Google-Smtp-Source: ACJfBotO47sQtoqCx1htAM3UrZOStPm8GxwM1ucw4IwEM8xQimn8NA23TVgzAmdA/CrAXd941dPTJw== X-Received: by 10.80.171.165 with SMTP id u34mr19887355edc.167.1516100096708; Tue, 16 Jan 2018 02:54:56 -0800 (PST) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id o30sm1282371eda.87.2018.01.16.02.54.55 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 16 Jan 2018 02:54:55 -0800 (PST) Date: Tue, 16 Jan 2018 11:54:43 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: Ferruh Yigit , Thomas Monjalon , dev@dpdk.org, stephen@networkplumber.org, Adrien Mazarguil Message-ID: <20180116105443.llmifnjgp2ntov7s@bidouze.vm.6wind.com> References: <20171222173846.20731-1-adrien.mazarguil@6wind.com> <1515509253-17834-1-git-send-email-matan@mellanox.com> <1515509253-17834-3-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1515509253-17834-3-git-send-email-matan@mellanox.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v3 2/8] net/failsafe: add "fd" parameter 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: , X-List-Received-Date: Tue, 16 Jan 2018 10:54:57 -0000 Hi Matam, Adrien, On Tue, Jan 09, 2018 at 02:47:27PM +0000, Matan Azrad wrote: > From: Adrien Mazarguil > > This parameter enables applications to provide device definitions through > an arbitrary file descriptor number. Ok on the principle, > @@ -161,6 +165,73 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params, > } > > static int > +fs_read_fd(struct sub_device *sdev, char *fd_str) > +{ > + FILE *fp = NULL; > + int fd = -1; > + /* store possible newline as well */ > + char output[DEVARGS_MAXLEN + 1]; > + int err = -ENODEV; > + int ret; ret is used as flag older, line counter and then error reporting. err should be the only variable used for reading errors from function and reporting it. It would be clearer to use descriptive names, such as "oflags" and "nl" or "lcount". I don't really care about one additional variable in this function, for the sake of expressiveness. > + > + RTE_ASSERT(fd_str != NULL || sdev->fd_str != NULL); > + if (sdev->fd_str == NULL) { > + sdev->fd_str = strdup(fd_str); > + if (sdev->fd_str == NULL) { > + ERROR("Command line allocation failed"); > + return -ENOMEM; > + } > + } > + errno = 0; > + fd = strtol(fd_str, &fd_str, 0); > + if (errno || *fd_str || fd < 0) { > + ERROR("Parsing FD number failed"); > + goto error; > + } > + /* Fiddle with copy of file descriptor */ > + fd = dup(fd); > + if (fd == -1) > + goto error; > + ret = fcntl(fd, F_GETFL); oflags = fcntl(...); > + if (ret == -1) > + goto error; > + ret = fcntl(fd, F_SETFL, fd | O_NONBLOCK); err = fcntl(fd, F_SETFL, oflags | O_NONBLOCK); Using (fd | O_NONBLOCK) is probably a mistake. > + if (ret == -1) > + goto error; > + fp = fdopen(fd, "r"); > + if (!fp) > + goto error; > + fd = -1; > + /* Only take the last line into account */ > + ret = 0; > + while (fgets(output, sizeof(output), fp)) > + ++ret; lcount = 0; while (fgets(output, sizeof(output), fp)) ++lcount; > + if (feof(fp)) { > + if (!ret) > + goto error; > + } else if (ferror(fp)) { > + if (errno != EAGAIN || !ret) > + goto error; > + } else if (!ret) { > + goto error; > + } These branches seems needlessly complicated: if (lcount == 0) goto error; else if (ferror(fp) && errno != EAGAIN) goto error; > + /* Line must end with a newline character */ > + fs_sanitize_cmdline(output); > + if (output[0] == '\0') > + goto error; > + ret = fs_parse_device(sdev, output); > + if (ret) > + ERROR("Parsing device '%s' failed", output); > + err = ret; no need to use ret instead of err here? err = fs_parse_device(sdev, output); if (err) ERROR("Parsing device '%s' failed", output); Thus allowing to remove the "ret" variable completely. > +error: > + if (fp) > + fclose(fp); > + if (fd != -1) > + close(fd); > + return err; > +} > + > +static int > fs_parse_device_param(struct rte_eth_dev *dev, const char *param, > uint8_t head) > { > @@ -202,6 +273,14 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params, > } > if (ret) > goto free_args; > + } else if (strncmp(param, "fd", 2) == 0) { How about strncmp(param, "fd(", 3) == 0 here? I think I made a mistake for dev and exec device types, no reason at this point to reiterate for fd as well. > + ret = fs_read_fd(sdev, args); > + if (ret == -ENODEV) { > + DEBUG("Reading device info from FD failed"); > + ret = 0; > + } > + if (ret) > + goto free_args; > } else { > ERROR("Unrecognized device type: %.*s", (int)b, param); > return -EINVAL; > @@ -409,6 +488,8 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params, > FOREACH_SUBDEV(sdev, i, dev) { > free(sdev->cmdline); > sdev->cmdline = NULL; > + free(sdev->fd_str); > + sdev->fd_str = NULL; > free(sdev->devargs.args); > sdev->devargs.args = NULL; > } > @@ -424,7 +505,8 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, const char *params, > param[b] != '\0') > b++; > if (strncmp(param, "dev", b) != 0 && > - strncmp(param, "exec", b) != 0) { > + strncmp(param, "exec", b) != 0 && > + strncmp(param, "fd", b) != 0) { If the strncmp above is modified, this one should be as well for consistency. -- Gaëtan Rivet 6WIND