Pointer constification

[ac000]

This patch series marks various pointer function arguments as const in the configuration sub-system.

The main impetus of this was to convert a bunch of

static nxt_str_t ...

variable declarations to

static const nxt_str_t ...

Having these only static seems a bit weird, given what that means and that they we're really being treated as string literals.

Making them static const really makes them const, and remove the thread safety issue, in that if you try to write to them then the compiler will yell at you!

There is also precedent for this in Unit as we already had

src/nxt_conf_validation.c:2032:    static const nxt_str_t  wsgi = nxt_string("wsgi");
src/nxt_conf_validation.c:2033:    static const nxt_str_t  asgi = nxt_string("asgi");
src/nxt_conf_validation.c:2424:    static const nxt_str_t  http = nxt_string("http");
src/nxt_conf_validation.c:2425:    static const nxt_str_t  https = nxt_string("https");
src/nxt_controller.c:513:    static const nxt_str_t json = nxt_string(
src/nxt_controller.c:1281:    static const nxt_str_t empty_obj = nxt_string("{}");
src/nxt_h1proto.c:865:    static const nxt_str_t tmp_name_pattern = nxt_string("/req-XXXXXXXX");
src/nxt_h1proto.c:1230:    static const nxt_str_t  connection[3] = {
src/nxt_http_variables.c:521:    static const nxt_str_t  connection[3] = {
src/nxt_php_sapi.c:196:    static const nxt_str_t  chdir = nxt_string("chdir");
src/nxt_router.c:273:static const nxt_str_t http_prefix = nxt_string("HTTP_");
src/nxt_router.c:274:static const nxt_str_t empty_prefix = nxt_string("");

Why?

Well it helps improve the code quality by helping to reduce programmer errors, by enabling the compiler to warn you when you are trying to modify something you shouldn't.

As Linus said

• Anything that can take a const pointer should always do so.

With the current fad of C bashing about it being unsafe, let's show the doom-sayers that there are things we can do in C (and more so with modern C) that can help eliminate whole classes of problems and this is just one small part of that.

[ac000]

Added the (u_char *) casts in nxt_conf_get_string()

$ git range-diff 996e7c78...0a8ef824
1:  996e7c78 ! 1:  0a8ef824 configuration: Constify more pointers
    @@ Commit message
         While it takes a value parameter that is never modified, simply making
         it const results in

    -      warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    +      CC     build/src/nxt_conf.o
    +    src/nxt_conf.c: In function ‘nxt_conf_get_string’:
    +    src/nxt_conf.c:170:20: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
    +      170 |         str->start = value->u.str.start;
    +          |                    ^

         due to the assignment operator. Making value const will allow for
         numerous other constification and seeing as we are not modifying it,
         seems worthwhile.

    -    So to get around the warning we simply take a non-const copy of value.
    +    We can get around the warning by casting ->u.{str,string}.start

         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

    @@ src/nxt_conf.c: nxt_conf_json_indentation(u_char *p, uint32_t level)
     -nxt_conf_get_string(nxt_conf_value_t *value, nxt_str_t *str)
     +nxt_conf_get_string(const nxt_conf_value_t *value, nxt_str_t *str)
      {
    -+    nxt_conf_value_t *v = (nxt_conf_value_t *) value;
    -+
          if (value->type == NXT_CONF_VALUE_SHORT_STRING) {
    --        str->length = value->u.str.length;
    +         str->length = value->u.str.length;
     -        str->start = value->u.str.start;
    -+        str->length = v->u.str.length;
    -+        str->start = v->u.str.start;
    ++        str->start = (u_char *) value->u.str.start;

          } else {
    --        str->length = value->u.string.length;
    +         str->length = value->u.string.length;
     -        str->start = value->u.string.start;
    -+        str->length = v->u.string.length;
    -+        str->start = v->u.string.start;
    ++        str->start = (u_char *) value->u.string.start;
          }
      }

[ac000]

Fix some wrapping/indentation

$ git range-diff 0a8ef824...b4d2153a
1:  0a8ef824 ! 1:  b4d2153a configuration: Constify more pointers
    @@ src/nxt_conf.h: nxt_conf_value_t *nxt_conf_json_parse(nxt_mp_t *mp, u_char *star

     -NXT_EXPORT void nxt_conf_get_string(nxt_conf_value_t *value, nxt_str_t *str);
     -NXT_EXPORT nxt_str_t *nxt_conf_get_string_dup(nxt_conf_value_t *value,
    -+NXT_EXPORT void nxt_conf_get_string(const nxt_conf_value_t *value, nxt_str_t *str);
    ++NXT_EXPORT void nxt_conf_get_string(const nxt_conf_value_t *value,
    ++    nxt_str_t *str);
     +NXT_EXPORT nxt_str_t *nxt_conf_get_string_dup(const nxt_conf_value_t *value,
          nxt_mp_t *mp, nxt_str_t *str);
      NXT_EXPORT void nxt_conf_set_string(nxt_conf_value_t *value,

[alejandro-colomar]

Hi Andrew!

This is placebo.

Nacked-by: Alejandro Colomar <alx@kernel.org>

alx@debian:~/tmp/c$ cat const.c 
#include <stdio.h>
#include <string.h>

struct s {
    int len;
    char *s;
};

static const struct s s = {3, "foo"};

int
main(void)
{
    memcpy(s.s, "bar", 3);
    putchar(s.s[0]);
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c 
alx@debian:~/tmp/c$ clang -Weverything const.c 
const.c:6:8: warning: padding struct 'struct s' with 4 bytes to align 's' [-Wpadded]
        char *s;
              ^
1 warning generated.
alx@debian:~/tmp/c$ ./a.out 
Segmentation fault

I worked some time ago in a solution that seems to work, but F5 will need to pay me to disclose it.

(Edit) Hidden off-topic discussion

Edit: The following is kept for historical reasons, but @callahad addressed them. It was a misunderstanding caused by corporate Outlook. The new manager of the project, @callahad, abused my trust, by asking me to help, by reviewing a CLA draft that F5 was considering applying to this project. This was in the context of talks about my contributions to the project, after he showed interest in them, and in resolving the conflicts initiated by the current "senior" engineer of the project. Of course, I helped, assuming good faith. He ghosted me after he got some value for free. In any case, considering how the management and senior staff of this project disregards the danger of Undefined Behavior, and unsafe code, mainly due to incompetence (see for an obvious example, where the fix is trivial, and the staff can't even acknowledge the bug, more than a year after), I don't think there's interest in solving problems like this one. I'd like to be wrong, though, so please anyone correct me.

[callahad]

He ghosted me after he got some value for free.

Hi @alejandro-colomar, I have no idea what you're talking about. I received no further communication from you and assumed you were occupied with other things. :-/ Sounds like something has been dropped, but I don't know where. Regardless, I'm sincerely sorry that you feel like I've ghosted you. Not my intent; let's cure that.

mainly due to incompetence

I must ask you to refrain from disparaging folks here. #795 hasn't been touched in well over a year, significantly before my time. We need to thoroughly re-triage, but righting this ship takes time. We'll get there.

[callahad]

Update: Corporate Outlook was flagging Alex's replies to me as Spam. 🤦‍♂️ I've followed up out-of-band.

Let's keep this review focused on this patch.

[alejandro-colomar]

Update: Corporate Outlook was flagging Alex's replies to me as Spam. 🤦‍♂️ I've followed up out-of-band.

Let's keep this review focused on this patch.

I'm glad to see that it wasn't your fault. I'm sorry about the assumptions I made. If you want, I can drop my comment, or just leave it for historical reasons.

[alejandro-colomar]

Hi Andrew!

The thing with constifying nxt_str_t is that you're constifying the pointer within the structure, not the pointee. There's no way in C to constify the pointee. You'd need to use a distinct structure type.

So, your patch protects against pointing to a different string (s->start = "foo";) but not against overwriting the string (memcpy(s->start, "foo", 3);).

Have a lovely night! Alex

[ac000]

Hi Andrew!

Hi Alex,

This is placebo.

Heh, if the below was really happening then it's worse than that!

Nacked-by: Alejandro Colomar <alx@kernel.org>

alx@debian:~/tmp/c$ cat const.c 
#include <stdio.h>
#include <string.h>

struct s {
  int len;
  char *s;
};

static const struct s s = {3, "foo"};

int
main(void)
{
  memcpy(s.s, "bar", 3);
  putchar(s.s[0]);
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c 
alx@debian:~/tmp/c$ clang -Weverything const.c 
const.c:6:8: warning: padding struct 'struct s' with 4 bytes to align 's' [-Wpadded]
        char *s;
              ^
1 warning generated.
alx@debian:~/tmp/c$ ./a.out 
Segmentation fault

Indeed, I am actually aware that modifying something defined as const behind its back is undefined behaviour...

I've re-checked where I've added consts and I don't see anywhere we try to modify them indirectly.

However your demo program segfaults for other reasons. I.e it still segfaults if you remove the static const

It segfaults on the memcpy(3) due to I think trying to overwrite the string literal "foo".

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000401055 in main () at const.c:13
13              memcpy(s.s, "bar", 3);

valgrind shows

==129333== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==129333==  Bad permissions for mapped region at address 0x402010
==129333==    at 0x401050: main (const.c:13)
==129333== 

However, I think I know what you were trying to demonstrate...

[ac000]

The thing with constifying nxt_str_t is that you're constifying the pointer within the structure, not the pointee. There's no way in C to constify the pointee. You'd need to use a distinct structure type.

Yes, I'm aware.

So, your patch protects against pointing to a different string (s->start = "foo";) but not against overwriting the string (memcpy(s->start, "foo", 3);).

Yes, but also no, as in it's not so much about preventing it from being modified, you can always access it through a non-const alias.

As we know, C simply has no concept of const meaning that the underlying memory is immutable.

What this is really about is type checking. It's about informing the compiler of your intentions.

The compiler can then warn you if you deviate from that (to a point, obviously if you're determined you can get around it, but then you're definitely shooting yourself in the foot).

So as Linus says, anything that can be const should be, to give the compiler more opportunity to help you. Of course it's best to think about this upfront because trying to do this retroactively is a minefield.

So that's what this patch set is for, not really about preventing things from being modified, but about enabling more compiler warnings if you try to do something unintentional.

[alejandro-colomar]

Hi Andrew!

Hi Alex,

This is placebo.

Heh, if the below was really happening then it's worse than that!

...

alx@debian:~/tmp/c$ cat const.c 
#include <stdio.h>
#include <string.h>

struct s {
    int len;
    char *s;
};

static const struct s s = {3, "foo"};

int
main(void)
{
    memcpy(s.s, "bar", 3);
    putchar(s.s[0]);
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c 
alx@debian:~/tmp/c$ clang -Weverything const.c 
const.c:6:8: warning: padding struct 'struct s' with 4 bytes to align 's' [-Wpadded]
        char *s;
              ^
1 warning generated.
alx@debian:~/tmp/c$ ./a.out 
Segmentation fault

Indeed, I am actually aware that modifying something defined as const behind its back is undefined behaviour...

I've re-checked where I've added consts and I don't see anywhere we try to modify them indirectly.

No, you don't modify the string literals. (Or at least not that I've seen.)

However your demo program segfaults for other reasons. I.e it still segfaults if you remove the static const

It segfaults on the memcpy(3) due to I think trying to overwrite the string literal "foo".

Yep.

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000401055 in main () at const.c:13
13              memcpy(s.s, "bar", 3);

valgrind shows

==129333== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==129333==  Bad permissions for mapped region at address 0x402010
==129333==    at 0x401050: main (const.c:13)
==129333== 

However, I think I know what you were trying to demonstrate...

Yep, I assume you know it. :)

Just to remove doubt: I was trying to demonstrate that this use of const does not protect against modifying the strings (which is the main reason one would want to use it, after all).

if you try to write to them then the compiler will yell at you!

So, it won't really yell at you. Yeah, if you modify the pointer to point to something else, or the length (admittedly, one will usually modify the length when modifying the string), it'll yell at you. And yeah, that's already an improvement. But only a mild improvement, not really protecting against a simple modification of the string, which is why I called it a placebo.

[ac000]

Just to remove doubt: I was trying to demonstrate that this use of const does not protect against modifying the strings (which is the main reason one would want to use it, after all).

I think I covered this is in my other reply. But C simply no such thing. const is really about type checking not preventing things being modified.

I didn't really understand what const was actually for until I read this.

[alejandro-colomar]

The thing with constifying nxt_str_t is that you're constifying the pointer within the structure, not the pointee. There's no way in C to constify the pointee. You'd need to use a distinct structure type.

Yes, I'm aware.

So, your patch protects against pointing to a different string (s->start = "foo";) but not against overwriting the string (memcpy(s->start, "foo", 3);).

Yes, but also no, as in it's not so much about preventing it from being modified, you can always access it through a non-const alias.

As we know, C simply has no concept of const meaning that the underlying memory is immutable.

What this is really about is type checking. It's about informing the compiler of your intentions.

The compiler can then warn you if you deviate from that (to a point, obviously if you're determined you can get around it, but then you're definitely shooting yourself in the foot).

My point is that you don't need to use casts or other workarounds to tell the compiler that you're determined to shoot yourself in the foot in this case. In my example program, I enabled all warnings, used const, and still shoot myself in the foot. nxt_str_t is necessarily unsafe, as it is now. const can't make it safe.

So as Linus says, anything that can be const should be, to give the compiler more opportunity to help you. Of course it's best to think about this upfront because trying to do this retroactively is a minefield.

So that's what this patch set is for, not really about preventing things from being modified, but about enabling more compiler warnings if you try to do something unintentional.

Hmmm, I agree that adding const where possible is good, but I think this case needs a little bit more thought, because one may think that by adding const the compiler will warn when doing something stupid, and then it will be a bad surprise when one realizes that the compiler won't really warn.

So, while I agree to adding this const, I think this is just protecting half of the problem.

[ac000]

So, while I agree to adding this const, I think this is just protecting half of the problem.

Right, my impetus was to fix all the weird static nxt_str_t's, the initial patches allowed for that, the last patch was just some other low hanging fruit in the vicinity.

We of course could go much further, would have been much better if this was though about upfront, but here we are. I tried doing some other const stuff and very quickly you do down a rabbit hole af adding consts (and casts) throughout the code base...

[alejandro-colomar]

Here's a smaller example showing what's going on:

alx@debian:~/tmp/c$ cat const.c 
#include <stdio.h>
#include <string.h>

int
main(void)
{
    char *const s = "foo";

    memcpy(s, "bar", 3);
    // s = "baz";  /* error: assignment of read-only variable ‘s’ */
    putchar(s[0]);
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c 
alx@debian:~/tmp/c$ ./a.out 
Segmentation fault

This shows how having const in the pointer does not produce any warning (yeah, in the = line it does, but). Mainly because qualifiers are discarded when taking an rvalue (so when passing to any function), since we actually take a copy of the (pointer) value, not the (pointer) variable itself. So, there's really little type safety in this example program.

[alejandro-colomar]

ISO C relevant paragraph: http://port70.net/~nsz/c/c11/n1570.html#6.3.2.1p2

Except when it is the operand of the sizeof operator, ..., or the left operand
of the . operator or an assignment operator, an lvalue ... is converted to
the value stored in the designated object (and is no longer an lvalue); this
is called lvalue conversion.  If the lvalue has qualified type, the value has
the unqualified version of the type of the lvalue; ...

[ac000]

I think that's still wrong... "foo" is still going to be in read-only memory... You'd still get the segfault without the const. I think you'd need to make it point to some malloc'd memory for it to "work"...

[alejandro-colomar]

I think that's still wrong... "foo" is still going to be in read-only memory... You'd still get the segfault without the const. I think you'd need to make it point to some malloc'd memory for it to "work"...

Oh yeah, the segfault is on purpose. I want to show that I'm "accidentally" shooting myself in the foot, and the compiler is not yelling at me, because this use of const really provides no type safety (or just a little bit, to be fair).

[alejandro-colomar]

If you prefer it without a segfault, here's some example modifying a malloc'd string:

alx@debian:~/tmp/c$ cat const.c 
#include <stdio.h>
#include <string.h>

int
main(void)
{
    char *const  s = strdup("foo");

    printf("%s\n", strcpy(s, "bar"));
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c 
alx@debian:~/tmp/c$ ./a.out 
bar

No type-safety violations here, according to the compiler, despite the use of const.

[alejandro-colomar]

Relevant links:

• https://stackoverflow.com/a/1143321/6872717
• https://stackoverflow.com/questions/21476869/constant-pointer-vs-pointer-to-constant

[ac000]

If you prefer it without a segfault, here's some example modifying a malloc'd string:

Well, it's just that it wasn't because of any const thing that it was segfaulting...

alx@debian:~/tmp/c$ cat const.c 
#include <stdio.h>
#include <string.h>

int
main(void)
{
  char *const  s = strdup("foo");

  printf("%s\n", strcpy(s, "bar"));
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c 
alx@debian:~/tmp/c$ ./a.out 
bar

No type-safety violations here, according to the compiler, despite the use of const.

Sure, it's not a silver bullet...

[ac000]

Of course if you define s in probably the more common way (albeit with a different meaning) of

const char *s = strdup("foo");

then you do get the compiler warning

const.c: In function ‘main’:
const.c:9:31: warning: passing argument 1 of ‘strcpy’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    9 |         printf("%s\n", strcpy(s, "bar"));
      |                               ^
In file included from const.c:2:
/usr/include/string.h:141:39: note: expected ‘char * restrict’ but argument is of type ‘const char *’
  141 | extern char *strcpy (char *__restrict __dest, const char *__restrict __src)
      |                      ~~~~~~~~~~~~~~~~~^~~~~~

Yes, it still modifies s, but at least you're warned about it...

[alejandro-colomar]

Of course if you define s in probably the more common way (albeit with a different meaning) of

const char *s = strdup("foo");

then you do get the compiler warning

Indeed. But that's not what you're doing in this patch. What you're doing is apply const to the pointer, not the pointee.

alx@debian:~/tmp/c$ cat const.c 
#include <stdio.h>

#define type_str(x)                                                           \
(                                                                             \
    _Generic(x,                                                           \
        char *:        "'char *'",                                    \
        const char *:  "'const char *'",                              \
        default:       "other"                                        \
    )                                                                     \
)

struct str_t {
    long  len;
    char  *start;
};

struct rstr_t {
    long        len;
    const char  *start;
};

int
main(void)
{
    static const struct str_t  sc = {3, "foo"};
    static struct str_t        s = {3, "foo"};
    const struct str_t         c = {3, "foo"};
    struct str_t               x = {3, "foo"};

    printf("sc: %s\n", type_str(sc.start));
    printf("s: %s\n", type_str(s.start));
    printf("c: %s\n", type_str(c.start));
    printf("x: %s\n", type_str(x.start));

    struct rstr_t              r = {3, "foo"};

    printf("r: %s\n", type_str(r.start));
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c 
alx@debian:~/tmp/c$ ./a.out 
sc: 'char *'
s: 'char *'
c: 'char *'
x: 'char *'
r: 'const char *'

const.c: In function ‘main’:
const.c:9:31: warning: passing argument 1 of ‘strcpy’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    9 |         printf("%s\n", strcpy(s, "bar"));
      |                               ^
In file included from const.c:2:
/usr/include/string.h:141:39: note: expected ‘char * restrict’ but argument is of type ‘const char *’
  141 | extern char *strcpy (char *__restrict __dest, const char *__restrict __src)
      |                      ~~~~~~~~~~~~~~~~~^~~~~~

Yes, it still modifies s, but at least you're warned about it...

But you don't have that kind of const in this patch, so you won't get these warnings.

[alejandro-colomar]

Or put a different way:

const struct a { char         *x; } A;
struct b       { char *const  x; }  B;
struct c       { const char   *y; } C;

The following two are equivalent: A.x and B.x are both char *const (which easily degrades to char * after undergoing lvalue conversion (see ISO C reference a few comments above). C.x is const char *, which is the safe thing; that const is enforced by the compiler.

[ac000]

Not in that exact case, no, but in the general case where you pass a const pointer into a function then pass it non-const into another.

[alejandro-colomar]

Not in that exact case, no, but in the general case where you pass a const pointer into a function then pass it non-const into another.

A const pointer isn't safe at all.

alx@debian:~/tmp/c$ cat const.c 
#include <string.h>

void f(char *p);

void g(char *const q)
{
    strcpy(q, "foo");
}
alx@debian:~/tmp/c$ cc -Wall -Wextra const.c -S
alx@debian:~/tmp/c$ 

What is safe is a const pointee (i.e., pointer to const).

Your calls having const nxt_str_t *s protect against passing to other functions accepting nxt_str_t *s, but you can easily change the prototype of those to use const nxt_str_t *s, and there won't be any warnings at all.

I suggest you do the following:

Apply a patch on top of your suggested patch, modifying a string inside a function that you modified (to supposedly add warnings against that). You can for example modify the first byte of a string to be /. And then show me any warnings you get. I bet there won't be any. So, please show what warnings this patch is adding, with real examples.

[ac000]

I think you're arguing for arguments sake now, my final reply to you is this...

1) Those nxt_str_t's should not be just statics, that makes no sense. static const does and matches others in the code. 2) Using unit idioms

Current way

#include <stdio.h>
#include <stddef.h>

typedef struct {
        size_t  length;
        char    *start;
} nxt_str_t;

#define nxt_length(s)           (sizeof(s) - 1)
#define nxt_string(str)         { nxt_length(str), (char *)str }

#define nxt_str_set(str, text)                                                \
        do {                                                                  \
                (str)->length = nxt_length(text);                             \
                (str)->start = (char *)text;                                  \
        } while (0)

int main(void)
{
        static nxt_str_t success_str = nxt_string("success");

        printf("%s\n", success_str.start);

        nxt_str_set(&success_str, "test");
        printf("%s\n", success_str.start);

        return 0;
}

$ gcc -Wall -Wextra -O2 -g -o s2 s2.c
$ ./s2
success
test

With

static const nxt_str_t success_str = nxt_string("success");

$ gcc -Wall -Wextra -O2 -g -o s2 s2.c
s2.c: In function ‘main’:
s2.c:14:31: error: assignment of member ‘length’ in read-only object
   14 |                 (str)->length = nxt_length(text);                             \
      |                               ^
s2.c:24:9: note: in expansion of macro ‘nxt_str_set’
   24 |         nxt_str_set(&success_str, "test");
      |         ^~~~~~~~~~~
s2.c:15:30: error: assignment of member ‘start’ in read-only object
   15 |                 (str)->start = (char *)text;                                  \
      |                              ^
s2.c:24:9: note: in expansion of macro ‘nxt_str_set’
   24 |         nxt_str_set(&success_str, "test");
      |         ^~~~~~~~~~~

I'm sure there's ways of defeating it, but this is definitely an improvement.

Anyway it accomplished my goal, so job done...

[alejandro-colomar]

I think you're arguing for arguments sake now,

Not really.

1. Those nxt_str_t's should not be just statics, that makes no sense. static const does and matches others in the code.

Okay.

2. Using unit idioms

Current way

#include <stdio.h>
#include <stddef.h>

typedef struct {
        size_t  length;
        char    *start;
} nxt_str_t;

#define nxt_length(s)           (sizeof(s) - 1)
#define nxt_string(str)         { nxt_length(str), (char *)str }

#define nxt_str_set(str, text)                                                \
        do {                                                                  \
                (str)->length = nxt_length(text);                             \
                (str)->start = (char *)text;                                  \
        } while (0)

int main(void)
{
        static nxt_str_t success_str = nxt_string("success");

        printf("%s\n", success_str.start);

        nxt_str_set(&success_str, "test");
        printf("%s\n", success_str.start);

        return 0;
}

$ gcc -Wall -Wextra -O2 -g -o s2 s2.c
$ ./s2
success
test

With

static const nxt_str_t success_str = nxt_string("success");

$ gcc -Wall -Wextra -O2 -g -o s2 s2.c
s2.c: In function ‘main’:
s2.c:14:31: error: assignment of member ‘length’ in read-only object
   14 |                 (str)->length = nxt_length(text);                             \
      |                               ^
s2.c:24:9: note: in expansion of macro ‘nxt_str_set’
   24 |         nxt_str_set(&success_str, "test");
      |         ^~~~~~~~~~~
s2.c:15:30: error: assignment of member ‘start’ in read-only object
   15 |                 (str)->start = (char *)text;                                  \
      |                              ^
s2.c:24:9: note: in expansion of macro ‘nxt_str_set’
   24 |         nxt_str_set(&success_str, "test");
      |         ^~~~~~~~~~~

Hmmm, yeah, that's a valid concern. This patch is insufficient, but still good.

I'm sure there's ways of defeating it, but this is definitely an improvement.

Agreed.

Have a lovely day! Alex

[ac000]

const'd a couple of static struct's

$ git range-diff 76874b93...06644f92
1:  3faf1f04 ! 1:  33547a50 Constify a bunch of static local variables
    @@ src/nxt_conf_validation.c: nxt_conf_vldt_if(nxt_conf_validation_t *vldt, nxt_con

          if (nxt_conf_type(value) != NXT_CONF_STRING) {
              return nxt_conf_vldt_error(vldt, "The \"if\" must be a string");
    +@@ src/nxt_conf_validation.c: nxt_conf_vldt_action(nxt_conf_validation_t *vldt, nxt_conf_value_t *value,
    +     nxt_conf_value_t        *action;
    +     nxt_conf_vldt_object_t  *members;
    + 
    +-    static struct {
    ++    static const struct {
    +         nxt_str_t               name;
    +         nxt_conf_vldt_object_t  *members;
    + 
     @@ src/nxt_conf_validation.c: nxt_conf_vldt_pass(nxt_conf_validation_t *vldt, nxt_conf_value_t *value,
          nxt_int_t  ret;
          nxt_str_t  segments[3];
    @@ src/nxt_conf_validation.c: nxt_conf_vldt_app(nxt_conf_validation_t *vldt, nxt_st
     -    static nxt_str_t  type_str = nxt_string("type");
     +    static const nxt_str_t  type_str = nxt_string("type");

    -     static struct {
    +-    static struct {
    ++    static const struct {
              nxt_conf_vldt_handler_t  validator;
    +         nxt_conf_vldt_object_t   *members;
    + 
     @@ src/nxt_conf_validation.c: nxt_conf_vldt_php(nxt_conf_validation_t *vldt, nxt_conf_value_t *value,
      {
          nxt_conf_value_t  *targets;
2:  bd28f4fd = 2:  591ea805 php: Constify some local static variables
3:  b4d2153a = 3:  402c83c0 configuration: Constify more pointers
4:  76874b93 = 4:  06644f92 Tighten up some string arrays

[ac000]

@hongzhidao

I'm going to leave nxt_conf_get_path() (and the other one I can't remember) as they are for now...

I'm not planning any more additions to this particular pull-request.

[hongzhidao]

@hongzhidao

I'm going to leave nxt_conf_get_path() (and the other one I can't remember) as they are for now...

I'm not planning any more additions to this particular pull-request.

Makes sense. Feel free to ship it, LGTM.

[ac000]

@hongzhidao I'm going to leave nxt_conf_get_path() (and the other one I can't remember) as they are for now... I'm not planning any more additions to this particular pull-request.

Makes sense. Feel free to ship it, LGTM.

Thanks! @hongzhidao

Although I can't until you actually approve it! ;)

While I'm here, do I have your permission to add

Reviewed-by: Zhidao HONG <z.hong@f5.com>

to each commit?

[hongzhidao]

Sure, you are welcome to add it.

[ac000]

Rebase with master

$ git range-diff 06644f92...cd9c4e35
 -:  -------- >  1:  678c0568 Fixes: 64934e59f ("HTTP: Introduce quoted target marker in HTTP parsing") Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
 -:  -------- >  2:  a48fbc03 Add additional information to the README
 -:  -------- >  3:  d7ce3569 Elaborate on docker image differences
 -:  -------- >  4:  237a26aa wasm-wc: Bump the rustls crate from 0.21.10 to 0.21.11
 -:  -------- >  5:  3fbca6ca Fix some trailing whitespace and long lines in the README
 1:  b7e95efa =  6:  269459a8 configuration: Constify numerous pointers
 2:  33547a50 =  7:  5ed112ae Constify a bunch of static local variables
 3:  591ea805 =  8:  719c6d7a php: Constify some local static variables
 4:  402c83c0 =  9:  13cedde3 configuration: Constify more pointers
 5:  06644f92 = 10:  cd9c4e35 Tighten up some string arrays

[ac000]

Add Zhidao's Reviewed-by:'s

$ git range-diff cd9c4e35...b26c119f
1:  269459a8 ! 1:  e5bc299d configuration: Constify numerous pointers
    @@ Commit message
         are currently defined with 'static' storage class and turn them into
         'static const', which will be done in a subsequent patch.

    +    Reviewed-by: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_conf.c ##
2:  5ed112ae ! 2:  8f861cf4 Constify a bunch of static local variables
    @@ Commit message

         This handles core code conversion.

    +    Reviewed-by: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_conf_validation.c ##
3:  719c6d7a ! 3:  33c978cc php: Constify some local static variables
    @@ Commit message
         Not sure why static, as they were being treated more like string
         literals, let's actually make them constants (qualifier wise).

    +    Reviewed-by: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_php_sapi.c ##
4:  13cedde3 ! 4:  31cec908 configuration: Constify more pointers
    @@ Commit message

         We can get around the warning by casting ->u.{str,string}.start

    +    Reviewed-by: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_conf.c ##
5:  cd9c4e35 ! 5:  b26c119f Tighten up some string arrays
    @@ Commit message

         This is the normal way of declaring such things.

    +    Reviewed-by: Zhidao HONG <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_controller.c ##

[ac000]

@hongzhidao

I've re-requested your review as it can be hard to find the 'Approve button` again...

[callahad]

I'll hit the approve button since it's late for Zhidao and from his comments he's clearly happy with this change.

[ac000]

Thanks Dan!