From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?iso-8859-1?Q?Ga=EBtan?= Rivet <gaetan.rivet@6wind.com>
Subject: Re: [PATCH v3 2/8] net/failsafe: add "fd" parameter
Date: Tue, 16 Jan 2018 12:19:08 +0100
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-Transfer-Encoding: 8bit
Cc: Ferruh Yigit <ferruh.yigit@intel.com>,
 Thomas Monjalon <thomas@monjalon.net>, dev@dpdk.org,
 stephen@networkplumber.org, Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Return-path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Tue, 16 Jan 2018 12:19:21 +0100 (CET)
Received: by mail-wr0-f195.google.com with SMTP id z48so14778810wrz.6
 for <dev@dpdk.org>; Tue, 16 Jan 2018 03:19:21 -0800 (PST)
Content-Disposition: inline
In-Reply-To: <20180116105443.llmifnjgp2ntov7s@bidouze.vm.6wind.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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 <adrien.mazarguil@6wind.com>
> > 
> > This parameter enables applications to provide device definitions through
> > an arbitrary file descriptor number.
> 
> Ok on the principle,
> 
> <snip>
> 
> > @@ -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