synchronous file output sink

[fenugrec]

As mentioned in #9 , here's a sync file output callback with both native Win and unix/linux implementations. Also includes a multi-threaded example, also for Win / nix. I didn't integrate it in the test suite because I wasn't sure how.

Implementation notes :

• single-process only due to the type of mutex used
• the output callback holds a mutex while calling the fwrite / WriteFile backend function, which may add latency if the OS implementation blocks. This doesn't seem to be a real problem in practice.
• CMake: to compile the example, I had to enable C_EXTENSIONS (so gcc gets "-std=gnu99" instead of "c99" . Otherwise I run into troubles due to _POSIX_SOURCE and clock_nanosleep() etc. I'm open to suggestions to improve this, though

I can rebase / squash the commits if required.

[wonder-mice]

Hi, thanks for working on this. I'll need some to go through this, but here initial thoughts after quick look:

• It's not completely clear to me what's best - to have syncs as option for the main library or a separate library. I like to to have core library to contain absolute minimum, so any additions can be made on top of it. When compilation/build options are used it's too tempting to modify core library instead of improving its interface, so other things can be built on top.
• You see logic behind your approach to provide opaque setup function that configure zf_log output machinery. However, I think it will be more flexible to provide functions that can fill in zf_log_output structure that can be used later with zf_log_set_output_p(). I was thinking about something like int zf_log_file_sync_init(struct zf_log_output *log) and int zf_log_file_sync_deinit(struct zf_log_output *log).
• There is no need for muteness, it's the same as with default syncs - OS will provide required synchronization (because of chosen buffer size).

[fenugrec]

Thanks for the quick review.

I like to to have core library to contain absolute minimum, so any additions can be made on top of it.

I thought that was exactly what I did ? i.e. I didn't touch zf_log.c ; to use the extra output sink one just needs to include the additional .h file and make sure to enable the right option at compile-time. But, at this stage it's pretty easy to change things so, whatever you prefer.

However, I think it will be more flexible to provide functions that can fill in zf_log_output structure that can be used later with zf_log_set_output_p(). [...]

I'm not sure if I understand what you mean by this; that would require modifying zf_log.c ? I looked briefly and decided not to touch it , since it's structured to use one global sink and I just want along with that model.

There is no need for muteness, it's the same as with default syncs - OS will provide required synchronization (because of chosen buffer size

I agree but I disagree. The mutex makes this robust on any kind of OS and load scenario with no possible doubt; there's just no real disadvantage. I'm sure the eventual performance bottleneck would be the OS write back-end, not the mutex overhead. If it somehow was a problem anyway, atomic primitives could be used instead.

[wonder-mice]

I thought that was exactly what I did

I was talking about having a separate library target, instead of inserting new code into existing zf_log library. Maybe zf_log_file_sync or something.

I'm not sure if I understand what you mean by this; that would require modifying zf_log.c?

No. It's just a different interface for a code you wrote. In your patch you initialize zf_log_output structure (well, not exactly, because you used zf_log_set_output_v() variant of the function, but you will if you switch to zf_log_set_output_p() variant) and then call zf_log_set_output_v() to set it. However, such approach will not work with aux family of logging functions that requires zf_log_spec to be specified explicitly (and zf_log_output is part of zf_log_spec). It's better to make you code to initialize zf_log_output structure and "return" it, so users can decide whether they want to set it with zf_log_set_output_p() or to use it with aux functions without changing current defaults.

The mutex makes this robust on any kind of OS.

Any OS that requires this mutex to work correctly is broken. Such OS must be addressed specifically, instead of making everybody else slower. To my current knowledge, all major OSes behave correctly.

I'm sure the eventual performance bottleneck would be the OS write back-end, not the mutex overhead.

Problem is not with overhead, but with the fact that one thread will be waiting while other is writing. Many OSes already has such mutex in much better place with clever optimization around it so on quick pass nobody waits. It's redundant.

[fenugrec]

WriteFile docs say

Although a single-sector write is atomic, a multi-sector write is not guaranteed to be atomic

to me, that alone is enough justification to keep the mutex in there, because I don't want to assume / guess that it will work as expected. In POSIX, fwrite() is "AS-unsafe" - same situation. Even though testing ( https://www.notthewizard.com/2014/06/17/are-files-appends-really-atomic/ ) shows that indeed it can be atomic up to 4K chunks, and arguably log entries will not be that long, that's not my point. If there's no way to guarantee or verify async safety, the mutex stays.

I was talking about having a separate library target [...] Maybe zf_log_file_sync

Hmm, so multiple librairies are produced ? If the sinks just contain setup functions and callbacks (and no dependency / calls to zf_log.c core funcs), I guess that could work yes. My use case is to statically link against it so either way doesn't matter. The code is so small that I would see no point in having a system-wide lib.

It's better to make you code to initialize zf_log_output structure and "return" it, so users can decide whether they want to set it with zf_log_set_output_p()

Agreed... I remember having had some problems with that, but now I don't remember why or what... arg

PS, unrelated: I think zf_log.c could be split into smaller pieces; for instance a lot of the #defines could go in a private .h file. I'm thinking of the PP and _ZF_LOG_MESSAGEFORMAT macros in particular