Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 26 Sep 2020 17:16:31 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL
 and GPIO_V2_LINE_GET_VALUES_IOCTL
Message-ID: <20200926091631.GA89482@sol>
References: <20200922023151.387447-1-warthog618@gmail.com>
 <20200922023151.387447-8-warthog618@gmail.com>
 <CAHp75VdQUbDnjQEr5X5q6WdU6rD=uBNznNn5=Vy=pvdwVj_hEA@mail.gmail.com>
 <20200924080921.GE17562@sol>
 <CAHp75VehvUTt19sBxgPTZszUmxDGZwqGAV7bgW5jVM8Mf63UJA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75VehvUTt19sBxgPTZszUmxDGZwqGAV7bgW5jVM8Mf63UJA@mail.gmail.com>
Precedence: bulk

On Fri, Sep 25, 2020 at 01:06:02PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 24, 2020 at 11:09 AM Kent Gibson <warthog618@gmail.com> wrote:
> > On Wed, Sep 23, 2020 at 02:11:54PM +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@gmail.com> wrote:
> 

...

> > > > +       /* Make sure this is terminated */
> > > > +       ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
> > > > +       if (strlen(ulr.consumer)) {
> > > > +               lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
> > > > +               if (!lr->label) {
> > > > +                       ret = -ENOMEM;
> > > > +                       goto out_free_linereq;
> > > > +               }
> > > > +       }
> > >
> > > Still don't get why we can\t use kstrndup() here...
> > >
> >
> > I know ;-).
> >
> > Another one directly from v1, and the behaviour there is to leave
> > lr->label nulled if consumer is empty.
> > It just avoids a pointless malloc for the null terminator.
> 
> Again, similar as for bitmap API usage, if it makes code cleaner and
> increases readability, I will go for it.
> Also don't forget the army of janitors that won't understand the case
> and simply convert everything that can be converted.
> 

Hmmm, there is more to it than I thought - gpiod_request_commit(),
which this code eventually calls, interprets a null label (not an
empty string) as indicating that the user has not set the label, in
which case it will set the desc label to "?". So userspace cannot
force the desc label to be empty.

We need to keep that label as null in that case to maintain that
behaviour.

I will add a comment there though.

Hmmm, having said that, does this form work for you:

	if (ulr.consumer[0] != '\0') {
		/* label is only initialized if consumer is set */
		lr->label = kstrndup(ulr.consumer, sizeof(ulr.consumer) - 1, GFP_KERNEL);
        ...

It actually compiles slightly larger, I guess due to the extra parameter
in the kstrndup() call, but may be slightly more readable??

Cheers,
Kent.