Notify handle - Githubissues

sdaoden commented 2 years ago

Funnily exactly 2 years after my private-mail attempt, now also on github (though i do not like it, just to lessen burden on people like you), here is the notify script patch again. Note i have split the test out into a separate commit, i have not tried the test ever since. The code i use ever since, too.

sdaoden commented 2 years ago

Merde. I try to look what has changed in the test series.

sdaoden commented 2 years ago

Ok now the tests pass with the new test framework. I had forgotten the commented off-by-one (in my code) that was still present in the original .. i fixed it and removed the comment. Otherwise identical to the first series.

troglobit commented 2 years ago

Hi, sorry for the late reply on this. I've been swamped with other work and also been changing $DAYJOB, so haven't had much time or energy to go through all PRs and issues on GitHub until now. Also, as you mention, we have a bit of history from before and I've honestly been holding off reviewing this much because of that.

Now, I've checked out this your initial branch, looked over the code changes and test. Besides from the actual review (coming up), I have the following general feedback:

Overall it looks good (!), even though I'm still not a fan of calling external scripts/programs from within syslogd. I'm leaning towards guarding it with a configure option --enable-notifiers, which would also help keep down the size a bit.
~I don't see you handling the SIGHUP case. E.g., if a user removes a notifier from their .conf file, the old one still lingers, right?~ -- My bad, missed the free() traversal in close_open_log_files().
You included a test, which is great, I wish all contributors did that. However, the test needs to be redone to:
- follow the structure of the other tests, i.e. use the FAIL(), SKIP(), etc. functions to ensure the correct return codes
- not start a second syslogd but use the first one, this may need some refactoring, which I can help out with since I might that for another test/feature coming up

I'll do a detailed review now, using the GitHub PR review.

sdaoden commented 2 years ago

Hello, thanks for looking. This is autotools right, i had to look, but surely could do this. I think i adjusted the test from other tests around, that second instance i mean, iirc, but of course i can try to improve this if you desire! Just keep on going.

sdaoden commented 2 years ago

I will look into this next week, ok? Thanks for doing this.

troglobit commented 2 years ago

Considering how long it took me to respond, of course! :)

That also gives me time to clean up the test code a bit. Got quite a ways tonight, will continue tomorrow.

sdaoden commented 2 years ago

Thanks again for your interest in adding this. On my github fork there is also the suggested notify_optional implementation in a branch of that name. I .. would rather not open a pull request, i find it makes the code ugly due to all the conditionals?

troglobit commented 2 years ago

On my github fork there is also the suggested notify_optional implementation in a branch of that name. I .. would rather not open a pull request, i find it makes the code ugly due to all the conditionals?

OK, I'll have a look later, but you''re probably right.

troglobit / sysklogd

Notify handle #45