From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by dpdk.org (Postfix) with ESMTP id 3C59412009 for ; Tue, 16 Jan 2018 12:19:21 +0100 (CET) Received: by mail-wr0-f195.google.com with SMTP id z48so14778810wrz.6 for ; Tue, 16 Jan 2018 03:19:21 -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=YiLQAbJz522AuUsR+CqSHE+Wb12q28LWfaxx21C7tRo=; b=sbjJOsmYrFr2HalBQJysc6scjBcTf22WJchIwnkaVNftyT+sFO2ZDowbMYqxGfXjZE GkMNBfyq5yo+Cp81Om1fCHebHCo0I2WFpRsp5WvOjRcBTj7LZBzXfdJTT3Ag6N1h2ZLF FKIbUus8GU80V1pK9ggCyvXRp4tA8Fix+hTSZeyWwvvd/g/nk0i7l+4QrUA8wQI0ZlLr AOoJEC1ZRqTdyYR18ETfoTFziEy8fkRyqJfdv4kcQwKpBDg0xXaGkGZDDBOdD2cQMSm6 FDjH9YtLR713Ak40TM3lsWmHW7gAqWPuFN3hHhJLoXatHchXmZPzJu508ZlmBlW59Omc +ENw== 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=YiLQAbJz522AuUsR+CqSHE+Wb12q28LWfaxx21C7tRo=; b=mpTVgVJuXDT3rog8JKpLLLWkZ2cwbRabejdZDEHjgn/voLXBMA+gfI9pngxoKlvfX1 Y2FcF1RGkg7N0+6MGvtm65jeEPbHA522gHCgHfns7NwOSB+SuXXqkDpYNRn8NXHJtV3G /mw6cKOgwpMFDmZRRhvaERnMdW4+L8E71vbYKasoqYCh1ISbFvr6V61LCwJyYdR8kcds bhfxREd//ox6q1lPo8npgiU549cEzGLnAw84Wev+GXoEoeD7tFm2YAqtuGnEPf7h6SII mTRGG9PkqY+bDWlDr+J1ECwz36PIajrsDbC96VuL2T2aUhw6nCYXKKQSPLI7pwWpmTpx HCtg== X-Gm-Message-State: AKwxytdjqowrmPDnbAZAr7lPKQlVWqSsHVW+I5c1s/RgIhiFNUBkaH2F CT3T/ftW0oANMNSnFF6+sDB9fQ== X-Google-Smtp-Source: ACJfBov0BB/yTYOxUX9CVmIt6+9DavHg4cC3s6HX0iaHABsDfSCJCVTaM8IvSE51xXUj3jrvPm3X3g== X-Received: by 10.223.163.153 with SMTP id l25mr11628434wrb.70.1516101560785; Tue, 16 Jan 2018 03:19:20 -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 h194sm5764423wma.8.2018.01.16.03.19.19 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 16 Jan 2018 03:19:20 -0800 (PST) Date: Tue, 16 Jan 2018 12:19:08 +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: <20180116111907.a2wi6kwv7g7bql5u@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> <20180116105443.llmifnjgp2ntov7s@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180116105443.llmifnjgp2ntov7s@bidouze.vm.6wind.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 11:19:21 -0000 Hi again, made a mistake in reviewing, see below. On Tue, Jan 16, 2018 at 11:54:43AM +0100, Gaëtan Rivet wrote: > 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. > This is sneaky. err is -ENODEV and would change to -1 on error, losing some meaning. > > + 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; > Here err would have been set to 0 previously with the fcntl call, meaning that jumping to error would return 0 as well. I know Adrien wanted to avoid the usual ugly if (error) { err = -ENODEV; goto error; } But this kind of sneakiness is not easy to parse and maintain. If someone adds a new path of error later, this kind of subtlety *will* be lost. So between ugliness and maintainability, I choose maintainability (being the maintainer, of course). -- Gaëtan Rivet 6WIND