Compile in C99 mode

[baonq-me]

Fix error when compiling in GCC 4.8.5 on CentOS 7

src/websocket.c: In function ‘ws_log_cmd’:
src/websocket.c:247:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
  for(int i = 0; p < eom && i < cmd->count; i++) {
  ^
src/websocket.c:247:2: note: use option -std=c99 or -std=gnu99 to compile your code
make: *** [src/websocket.o] Error 1

[nicolasff]

Hi @baonq-me,

Thanks for the pull request! I was able to reproduce the issue with this docker image of CentOS 7 and gcc.x86_64 version 4.8.5-44.el7.

That said, I don't think this change is quite right. -std=gnu99 is not the same as C99, see the description for it in the GCC docs:

-std=gnu99 (for C99 with GNU extensions)

From what I can tell, clang does not fully support -std=gnu99 and Webdis does not use any of these extensions either. -std=c99 would make sense though, as you point out this construct requires it.

Thinking about this change, it seems to me that it would make sense to do this where most of the compile flags are already set. The line you changed is there just to bring in CPPFLAGS as the comment above explains, so with your change this comment is no longer accurate.

You can put it here:

- CFLAGS ?= -Wall -Wextra -Isrc -Isrc/jansson/src -Isrc/http-parser -MD
+ CFLAGS ?= -std=c99 -Wall -Wextra -Isrc -Isrc/jansson/src -Isrc/http-parser -MD

Can you please make this change? I'll run the validation workflows once it's in.

[nicolasff]

Hmm, I see that -std=c99 is actually not enough to build on Centos 7:

src/server.c: In function 'server_daemonize':
src/server.c:203:61: error: 'O_NOFOLLOW' undeclared (first use in this function)
   int pid_fd = open(pidfile, O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0600);

If you're up for it, would you mind removing O_NOFOLLOW from this function call? It was only standardized in POSIX.1-2008 which is a bit too recent for my liking.

So please add this change to your pull request, in src/server.c:

-               int pid_fd = open(pidfile, O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0600);
+               int pid_fd = open(pidfile, O_WRONLY | O_CREAT | O_TRUNC, 0600);

With the -std=c99 change shown above and the removal of O_NOFOLLOW, I was able to build Webdis on the latest main (c6b9d5221365d489a15ce491911f2d10e9aed20e) albeit with some warnings. I'll look into those soon.

That said… one of these warnings was for src/slog.c, mentioning that localtime_r was not found. As the man page for it explains, this requires one of a few constants:

asctime_r(), ctime_r(), gmtime_r(), localtime_r(): _POSIX_C_SOURCE >= 1 || _XOPEN_SOURCE || _BSD_SOURCE || _SVID_SOURCE || _POSIX_SOURCE

It seems like Webdis actually requires one of these, and from what I can tell from other projects -D_POSIX_SOURCE seems appropriate.

The CFLAGS line then becomes, with both the -std=c99 change and the constant definition:

- CFLAGS ?= -Wall -Wextra -Isrc -Isrc/jansson/src -Isrc/http-parser -MD
+ CFLAGS ?= -std=c99 -Wall -Wextra -Isrc -Isrc/jansson/src -Isrc/http-parser -MD -D_POSIX_SOURCE

With ^ this change and the O_NOFOLLOW removal, I was able to at least start Webdis and run some tests successfully.

Wow, I never knew Webdis didn't even build on CentOS 7, although it's not particularly surprising that it was never reported given that it was released almost 9 years ago. In any case, I look forward to your updates if you're happy to make these changes, now 3 of them:

1. Add -std=c99 at the start of CFLAGS
2. Add -D_POSIX_SOURCE at the end
3. Remove O_NOFOLLOW from the open() call in src/server.c

Thanks again!

[nicolasff]

@baonq-me I've pulled in your commit here: 50d16b16ca4724373e02edffdfe08709dae52450 along with a couple more CentOS fixes. You should be able to build Webdis cleanly from master on CentOS 7, please let me know if this is not the case.

I've tested it with a Docker image for CentOS 7, Webdis builds and basic tests are passing.

Thanks again for reporting this issue! Testing on a 9-year-old OS was not really part of my release process. On this subject, I'll tag Webdis 0.1.21 soon which will include these changes as well as some security updates for Redis and a few dependencies.