| Message ID | 20200529153814.213125-5-alainm@chromium.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | Load default system configuration from file. | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. While we are preparing for reviewing the patches, we found the following issue/warning. Test Result: checkgitlint Failed Outputs: 1: T3 Title has trailing punctuation (.): "fixing const decoration warnins on options." --- Regards, Linux Bluetooth
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.
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
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
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;