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;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: [BlueZ PATCH v3 4/4] fixing const decoration warnins on options.
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <CALWDO_UoNKFcFgTNUgY691Lj61udG0WsL9vdxbSF4Q0fpqBwEg@mail.gmail.com>
Date:   Wed, 10 Jun 2020 10:55:10 +0200
Cc:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        Alain Michaud <alainm@chromium.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Transfer-Encoding: 8BIT
Message-Id: <B735F98B-8EB8-4FA3-B0C1-52A8C2CC8C60@holtmann.org>
References: <20200529153814.213125-1-alainm@chromium.org>
 <20200529153814.213125-5-alainm@chromium.org>
 <CABBYNZJJ751fxpjpZ0MFvUsQ21H9CptM_gySAgZbcGPwz76W2Q@mail.gmail.com>
 <CALWDO_UoNKFcFgTNUgY691Lj61udG0WsL9vdxbSF4Q0fpqBwEg@mail.gmail.com>
To:     Alain Michaud <alainmichaud@google.com>
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Alain,

>>> ---
>>> 
>>> 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.

I am a big fan of letting the compiler help us, but the readability concerns me a bit in this case. Can we explore what kind of macros we might be able to introduce to make this easier on the eyes.

Regards

Marcel