9 ms·

Props to the author for posting the "testimonials." Got a chuckle out of it. Did not look at the code.

by markbnj 11y ago

politician 11y ago
Right, yeah, well, now I'm not sure if this is a joke/troll project.
- generic_user 11y ago
  Its a real project. And there is a few other projects OpenRC etc. The longevity of Unix over the years has been its highly modular architecture. You can simply rip a daemon out and replace it with a new one. I don't think any attempt to create a monoculture will win favour in the long run. Even if Linux is utterly consumed the result would be a shift to Bsd.
nkurz 11y ago
At a glance, the code is clean and idiomatic C written by an experienced professional programmer. Comments are sparse and describe intent. Variable and function names are consistent and well chosen. I'd be happy giving it to a new programmer to use as a style guide. I have no idea if the architecture is suitable for the purpose, but anyone who is dismissing this based on code quality should be ignored. Representative Sample: https://github.com/ServiceManager/ServiceManager/blob/master/app/restartd/unit.c https://github.com/ServiceManager/ServiceManager/blob/master...
- frozenport 11y ago
  4/10 needs more RAII :P
- AndyKelley 11y ago
  I agree with the comment about overusing macros though. Instead of: #define DbgEnteringState(x) \ printf ("[%s] Unit entering state %s\n", unit->name, #x); How about: static void dbg_entering_state(unit_t * unit, const char * state_name) { fprintf (stderr, "[%s] Unit entering state %s\n", unit->name, state_name); } Friendlier to humans reading code, IDEs, and debuggers. I went ahead and fixed it writing to the wrong stdio stream too.
  - bcoates 11y ago
    That's clearly trace code intended to be defined away by the build system. It's idiomatic to make such things macros. Also, your way forces unnecessary verbosity onto the caller; passing unit by name was not a typo.
    - asynchronous13 11y ago
      SetOrExit ("S16.Name"); Oh wait, SetOrExit is a macro ... #define SetOrExit(Name) \ if (!prop_find_name (new_svc->properties, Name)) \ { \ OnError ("error: %s not set", #Name); \ } Oh wait, OnError is also a macro #define OnError(...) \ fprintf (stderr, __VA_ARGS__); \ goto on_error; There's a goto hidden in a doubly-nested macro? Sorry, that is not good practice.
      - kbenson 11y ago
        It's C style exception handling. Both the two macros, the place they are used, and the jump target all exist within the same 80 line file. I don't even write C and it's pretty obvious what's going on here. I might choose somewhat different names for the macros, but I'm not going to argue too much about that, it's a slippery slope.
        AndyKelley 11y ago
        Here's an alternate method of C exception handling: https://github.com/andrewrk/libsoundio/blob/4ce3429bdd9b08a01cb8f1b60c38a61dc5d818df/src/coreaudio.cpp#L411 https://github.com/andrewrk/libsoundio/blob/4ce3429bdd9b08a0... It does not use macros or goto and I find that it makes the code easy to reason locally about. In summary: struct RefreshDevices { // fields that require clean up } static void deinit_refresh_devices(struct RefreshDevices *rd) { // clean up rd regardless of what state it's in } static int refresh_devices(struct SoundIoPrivate *si) { struct RefreshDevices rd = {0}; if (error_occurred) { deinit_refresh_devices(&rd); return ErrorCode; } // ... deinit_refresh_devices(&rd); return 0; } Whenever the ownership of a resource changes to something other than the stack of refresh_devices, you just null it out and it won't get cleaned up. If calling deinit on every exit path of the function is too much typing you can wrap that function in another function that calls deinit, and the function doing the actual work can just return an error code without worrying about cleanup.
- cremno 11y ago
  1. naming conventions are unusual and inconsistent (List_destroy but then there's destroy_svcs_list) 2. the return value of malloc() et al. isn't checked (http://www.etalabs.net/overcommit.html http://www.etalabs.net/overcommit.html) 3. the register storage-class specifier is used multiple times (mainly lib/s16db/translate.c) 4. function declarations are missing a prototype (() instead of (void))
  - nkurz 11y ago
    I'm not claiming the code is perfect. I'm claiming that at a glance it looks like professional code of a quality similar that of other high quality C codebases: Linux, Python, Perl, Apache, sqlite, etc. One can always do better. I (and likely the author) disagree with you on the importance of checking the return value of malloc(). Because of overcommit (as you point out), a non-null value returned from malloc() does not mean that you will not crash when you access the pointer. If the pointer is used directly, crash on NULL might a reasonable approach. It's when NULL is retained and then used as the base of an array that it may become a security problem. Configuring malloc() to abort() on failure would be probably my preferred solution. I agree with the last point, but think it's a minor one. While I'd like C to treat () in a function definition as equivalent to (void), for historical reasons it does not. The author trades off the visual noise of the word 'void' for better error reporting. But depending on your compiler, you may still get a warning. On the computer I just tried, 'icc' and 'clang' gave clear warnings but 'gcc' did not. nate@ubuntu:~/C$ cat void.c #include <stdio.h> int empty() { return 3; } int main(void) { printf("%d\n", empty(4)); return 0; } nate@ubuntu:~/C$ icc -Wall -o void void.c void.c(3): warning #140: too many arguments in function call int main(void) { printf("%d\n", empty(4)); return 0; } ^ nate@ubuntu:~/C$ clang -Wall -o void void.c void.c:3:40: warning: too many arguments in call to 'empty' int main(void) { printf("%d\n", empty(4)); return 0; } ~~~~~ ^
    - AndyKelley 11y ago
      Not all systems have overcommit enabled. Notably, as the article linked above points out, robust systems do not have overcommit enabled. The OOM killer is heuristics based and cannot be relied on in robust systems. This means that libraries and applications that want to be viable on such platforms have to recognize that malloc may return NULL and respond accordingly.
joshumax 11y ago
Darn, it looks like I'm going to have to do an actual writeup on http://pid1.wikia.com/ http://pid1.wikia.com/ now for System XVI...
zobzu 11y ago
the use of macro is a little scary thou and not exactly typical C stuff. interesting to see where this will go.. also, the name is cool but XIV would fit the logo better than XVI ;)

Props to the author for posting the "testimonials." Got a chuckle out of it. Did not look at the code.

by markbnj 11y ago

Props to the author for posting the "testimonials." Got a chuckle out of it. Did not look at the code.

politician 11y ago

Right, yeah, well, now I'm not sure if this is a joke/troll project.

generic_user 11y ago
Its a real project. And there is a few other projects OpenRC etc. The longevity of Unix over the years has been its highly modular architecture. You can simply rip a daemon out and replace it with a new one. I don't think any attempt to create a monoculture will win favour in the long run. Even if Linux is utterly consumed the result would be a shift to Bsd.

nkurz 11y ago

At a glance, the code is clean and idiomatic C written by an experienced professional programmer. Comments are sparse and describe intent. Variable and function names are consistent and well chosen. I'd be happy giving it to a new programmer to use as a style guide. I have no idea if the architecture is suitable for the purpose, but anyone who is dismissing this based on code quality should be ignored. Representative Sample: https://github.com/ServiceManager/ServiceManager/blob/master/app/restartd/unit.c https://github.com/ServiceManager/ServiceManager/blob/master...

frozenport 11y ago
4/10 needs more RAII :P
AndyKelley 11y ago
I agree with the comment about overusing macros though. Instead of: #define DbgEnteringState(x) \ printf ("[%s] Unit entering state %s\n", unit->name, #x); How about: static void dbg_entering_state(unit_t * unit, const char * state_name) { fprintf (stderr, "[%s] Unit entering state %s\n", unit->name, state_name); } Friendlier to humans reading code, IDEs, and debuggers. I went ahead and fixed it writing to the wrong stdio stream too.
- bcoates 11y ago
  That's clearly trace code intended to be defined away by the build system. It's idiomatic to make such things macros. Also, your way forces unnecessary verbosity onto the caller; passing unit by name was not a typo.
  - asynchronous13 11y ago
    SetOrExit ("S16.Name"); Oh wait, SetOrExit is a macro ... #define SetOrExit(Name) \ if (!prop_find_name (new_svc->properties, Name)) \ { \ OnError ("error: %s not set", #Name); \ } Oh wait, OnError is also a macro #define OnError(...) \ fprintf (stderr, __VA_ARGS__); \ goto on_error; There's a goto hidden in a doubly-nested macro? Sorry, that is not good practice.
    - kbenson 11y ago
      It's C style exception handling. Both the two macros, the place they are used, and the jump target all exist within the same 80 line file. I don't even write C and it's pretty obvious what's going on here. I might choose somewhat different names for the macros, but I'm not going to argue too much about that, it's a slippery slope.
      - AndyKelley 11y ago
        Here's an alternate method of C exception handling: https://github.com/andrewrk/libsoundio/blob/4ce3429bdd9b08a01cb8f1b60c38a61dc5d818df/src/coreaudio.cpp#L411 https://github.com/andrewrk/libsoundio/blob/4ce3429bdd9b08a0... It does not use macros or goto and I find that it makes the code easy to reason locally about. In summary: struct RefreshDevices { // fields that require clean up } static void deinit_refresh_devices(struct RefreshDevices *rd) { // clean up rd regardless of what state it's in } static int refresh_devices(struct SoundIoPrivate *si) { struct RefreshDevices rd = {0}; if (error_occurred) { deinit_refresh_devices(&rd); return ErrorCode; } // ... deinit_refresh_devices(&rd); return 0; } Whenever the ownership of a resource changes to something other than the stack of refresh_devices, you just null it out and it won't get cleaned up. If calling deinit on every exit path of the function is too much typing you can wrap that function in another function that calls deinit, and the function doing the actual work can just return an error code without worrying about cleanup.
cremno 11y ago
1. naming conventions are unusual and inconsistent (List_destroy but then there's destroy_svcs_list) 2. the return value of malloc() et al. isn't checked (http://www.etalabs.net/overcommit.html http://www.etalabs.net/overcommit.html) 3. the register storage-class specifier is used multiple times (mainly lib/s16db/translate.c) 4. function declarations are missing a prototype (() instead of (void))
- nkurz 11y ago
  I'm not claiming the code is perfect. I'm claiming that at a glance it looks like professional code of a quality similar that of other high quality C codebases: Linux, Python, Perl, Apache, sqlite, etc. One can always do better. I (and likely the author) disagree with you on the importance of checking the return value of malloc(). Because of overcommit (as you point out), a non-null value returned from malloc() does not mean that you will not crash when you access the pointer. If the pointer is used directly, crash on NULL might a reasonable approach. It's when NULL is retained and then used as the base of an array that it may become a security problem. Configuring malloc() to abort() on failure would be probably my preferred solution. I agree with the last point, but think it's a minor one. While I'd like C to treat () in a function definition as equivalent to (void), for historical reasons it does not. The author trades off the visual noise of the word 'void' for better error reporting. But depending on your compiler, you may still get a warning. On the computer I just tried, 'icc' and 'clang' gave clear warnings but 'gcc' did not. nate@ubuntu:~/C$ cat void.c #include <stdio.h> int empty() { return 3; } int main(void) { printf("%d\n", empty(4)); return 0; } nate@ubuntu:~/C$ icc -Wall -o void void.c void.c(3): warning #140: too many arguments in function call int main(void) { printf("%d\n", empty(4)); return 0; } ^ nate@ubuntu:~/C$ clang -Wall -o void void.c void.c:3:40: warning: too many arguments in call to 'empty' int main(void) { printf("%d\n", empty(4)); return 0; } ~~~~~ ^
  - AndyKelley 11y ago
    Not all systems have overcommit enabled. Notably, as the article linked above points out, robust systems do not have overcommit enabled. The OOM killer is heuristics based and cannot be relied on in robust systems. This means that libraries and applications that want to be viable on such platforms have to recognize that malloc may return NULL and respond accordingly.

joshumax 11y ago

Darn, it looks like I'm going to have to do an actual writeup on http://pid1.wikia.com/ http://pid1.wikia.com/ now for System XVI...

zobzu 11y ago

the use of macro is a little scary thou and not exactly typical C stuff. interesting to see where this will go.. also, the name is cool but XIV would fit the logo better than XVI ;)