Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200529153814.213125-1-alainm@chromium.org> <20200529153814.213125-5-alainm@chromium.org>
 <CABBYNZJJ751fxpjpZ0MFvUsQ21H9CptM_gySAgZbcGPwz76W2Q@mail.gmail.com>
In-Reply-To: <CABBYNZJJ751fxpjpZ0MFvUsQ21H9CptM_gySAgZbcGPwz76W2Q@mail.gmail.com>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Mon, 1 Jun 2020 11:18:06 -0400
Message-ID: <CALWDO_UoNKFcFgTNUgY691Lj61udG0WsL9vdxbSF4Q0fpqBwEg@mail.gmail.com>
Subject: Re: [BlueZ PATCH v3 4/4] fixing const decoration warnins on options.
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Alain Michaud <alainm@chromium.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Luiz,

On Fri, May 29, 2020 at 1:17 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Fri, May 29, 2020 at 8:41 AM Alain Michaud <alainm@chromium.org> wrote:
> >
> > This changes fixes const decoration warnins on options.
>
> What us this fixing?
>
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  src/main.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/main.c b/src/main.c
> > index ca27f313d..cea50a3d2 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -80,7 +80,7 @@ static enum {
> >         MPS_MULTIPLE,
> >  } mps = MPS_OFF;
> >
> > -static const char *supported_options[] = {
> > +static const char * const supported_options[] = {
> >         "Name",
> >         "Class",
> >         "DiscoverableTimeout",
> > @@ -129,7 +129,7 @@ static const char * const controller_options[] = {
> >         NULL
> >  };
> >
> > -static const char *policy_options[] = {
> > +static const char * const policy_options[] = {
> >         "ReconnectUUIDs",
> >         "ReconnectAttempts",
> >         "ReconnectIntervals",
> > @@ -137,7 +137,7 @@ static const char *policy_options[] = {
> >         NULL
> >  };
> >
> > -static const char *gatt_options[] = {
> > +static const char * const gatt_options[] = {
> >         "Cache",
> >         "KeySize",
> >         "ExchangeMTU",
> > @@ -146,8 +146,8 @@ static const char *gatt_options[] = {
> >  };
> >
> >  static const struct group_table {
> > -       const char *name;
> > -       const char **options;
> > +       const char * const name;
> > +       const char * const * const options;
> >  } valid_groups[] = {
> >         { "General",    supported_options },
> >         { "Controller", controller_options },
> > @@ -243,7 +243,7 @@ static enum jw_repairing_t parse_jw_repairing(const char *jw_repairing)
> >
> >
> >  static void check_options(GKeyFile *config, const char *group,
> > -                                               const char **options)
> > +                                       const char * const * const options)
> >  {
> >         char **keys;
> >         int i;
> > --
> > 2.27.0.rc0.183.gde8f92d652-goog
> >
>
> I wonder how usufull is to tell it is a constant pointer to a constant
> character given that is so rarely in the kernel and userspace,
> something like const char * const * const would be very hard to keep
> it readable.

I'm personally a big fan of leveraging the compiler to find bugs, but
in this case it also allows the compiler to put all the strings into
read only sections.  In this example, it will catch code trying to
write into the string or prevent code execution in the read only
sections if there is ever a bug that leverages this data section to
store code that can be manipulated.  For readability, I've seen other
platforms use type definitions LPCSTR to typedef const char * const
etc..  I'm happy to follow guidance here.


>
> --
> Luiz Augusto von Dentz