Feature signal support

[james-knippes]

This adds support for sending signals to services, as I consider it a generally useful feature and implemented it with hacky scripts parsing the output of dinitctl status $service in the past. Many services use signals for reloading configuration files and/or enabling/disabling specific runtime features. The feature is currently requested by #134 and @davmac314 commented that

it's something that is planned

As this would be my first contribution to the project I appreciate any feedback. I'm willing to implement changes/improvements.

I've tried to follow contribution guidelines. Tests are still missing. I will add them if the feature is generally considered accepted.

• should follow existing style and conventions
• should keep the design considerations in mind (read the DESIGN document)
• should be accompanied by appropriate documentation additions/updates
• should include tests where practical

Open questions/limitations:

• Should signalling be moved from control.cc directly as a feature into the (decedents of the) service_record class ?
• I've found no way to translate signal names/abbrevations to their integer counterparts at runtime, that is system and architecture independent. The sigabbrev_np function is a gnu extension and only works for non realtime signals. Here is how kill from util-linux does it, but I'm not sure if this kind of conversion is worth it.

[mobin-2008]

Hello. Thanks for the PR. I did really quick review on it and I suggest to add DINIT_CP_SIGNAL as "new feature in Control protocol V2 (Dinit 0.17)" in line 15 of control.cc file.

Regards

[q66]

i think it would also be nice if there was a unified way to tell a service to reload its configuration, without sending an explicit signal; the service file could define the action (with choice between signal and command, e.g. reload-signal = hup or reload-command = something, default implicit behavior being sending SIGHUP)

unfortunately there is already dinitctl reload, just with a different meaning than what it means in most service managers...

[james-knippes]

Hello. Thanks for the PR. I did really quick review on it and I suggest to add DINIT_CP_SIGNAL as "new feature in Control protocol V2 (Dinit 0.17)" in line 15 of control.cc file.

Thanks . I've added DINIT_CP_SIGNAL to the list of new features.

[james-knippes]

i think it would also be nice if there was a unified way to tell a service to reload its configuration, without sending an explicit signal; the service file could define the action (with choice between signal and command, e.g. reload-signal = hup or reload-command = something, default implicit behavior being sending SIGHUP)

I also think this would be a very useful feature. And one that a lot of service managers and services support. Signalling services may have a lot different uses though. I use it e.g. for toggling the visibility of my onscreen keyboard with SIGRTMIN+0.

The (implementation details of a) service reloading feature probably deserves its own discussion, as it may impact future usability/expendability. I've started #178 for this purpose (:

[james-knippes]

Could someone point me to the right place and to which extend tests should be implemented for this feature?
I'm not too familiar with (c++) testing in general. I would guess in test.cc or proctests.cc, but i'm not sure how to test the handling of pid values, as they don't seem to get mocked in the test_service or base_process_service_test class ?

I may be missing some basics here and would appreciate hints for test examples for orientation or general resources/introductions to the topic.

[davmac314]

Sorry for taking a little while to get to looking at your PR properly, and thanks for offering a contribution!

The actual code changes look good (minus a few minor style inconsistencies eg spaces around the condition in if).

My main concern is that being forced to specify signals via number rather than name is awkward. There is already a signal_name_to_number function in load-service.h, which is already included by dinitctl.cc so would be straightforward to use.

Also the default of USR1 (in fact any default) is unlikely to be useful for most services, so it would be a better interface if the signal could just be specified without a switch, something like:

dinitctl signal <signal-name-or-number> <service-name>

You could potentially still allow just:

dinitctl signal <service-name>

... to send a default signal (USR1 or whatever) but in my opinion this isn't really necessary.

[davmac314]

Could someone point me to the right place and to which extend tests should be implemented for this feature?
I'm not too familiar with (c++) testing in general. I would guess in test.cc or proctests.cc, but i'm not sure how to test the handling of pid values, as they don't seem to get mocked in the test_service or base_process_service_test class ?

Probably proctests.cc.

Pids are mocked via test-bpsys.cc which also contains mocks for various system functions like kill. Within proctests.cc you can use last_sig_sent to check what signal has most recently been sent (there's currently no way to check whether it was sent to the correct pid, so you could not worry about that for now, or make changes in test-bpsys.cc so that it records the pid target in another variable, or implement an integration test instead).

[davmac314]

Probably proctests.cc.

Actually, since the only thing to really test is the control protocol handling, probably cptests.cc. The complication is that this test needs to use a process-based service which the cptests currently don't do. The proctests should give you an idea of what's necessary; you have to call base_process_service_test::exec_succeeded as part of the test to actually get the test service started. See test_proc_service_start() in proctests.cc for that.

[james-knippes]

@davmac314 thank you for the feedback.

The actual code changes look good (minus a few minor style inconsistencies eg spaces around the condition in if).

I've fixed the white-space inconsistencies.

Also the default of USR1 (in fact any default) is unlikely to be useful for most services, so it would be a better interface if the signal could just be specified without a switch, something like:

dinitctl signal <signal-name-or-number> <service-name>

You are totally right, the interface was rather awkward. I've update the the parameter handling accordingly

My main concern is that being forced to specify signals via number rather than name is awkward. There is already a signal_name_to_number function in load-service.h, which is already included by dinitctl.cc so would be straightforward to use.

I don't know how i could miss the existing signal_name_to_number function :D I've updated dinitctl to accept the signal names or numbers/integers. I've also changed the signal_name_to_number function to include all POSIX signals. System specific signals like Linux real-times signals have to be entered as integers.

I'll have a look at the tests in the next days.

[davmac314]

Hi @james-knippes - just wondering if you were still planning on finishing off this PR? I think that tests are the only thing pending

[davmac314]

I've reviewed this whole thing again and there are a number of things that should be changed or fixed before I'm happy to include this:

• first, let's not make a habit of pushing changes to someone else's repo, even if we do have that ability. In cases like this I think the best thing is to pull the changes from their repo, push them to your own, and open a new PR. That's ok for this time though.
• it looks like "dinitctl signal (signal-name) --list" wouldn't error out, but it should. Correct syntax is either "dinitctl signal (signal-name) (service-name)" or "dinitctl signal --list". Rather than converting the signal name to a number where it currently happens (during arg parsing), it should be done in the argument post-processing (same place it does the special checks for ENABLE_SERVICE, DISABLE_SERVICE, SETENV, etc). Also the check for whether the signal is a number (using std::stoi) doesn't currently check if there is junk text after the number, it should (eg it currently would accept "99junk" and treat it as 99 instead of giving an error).
• let's rename DINIT_RP_SIGNAL_BADPID. It's being used when the service doesn't have a PID, but that's not the same as a "bad" PID. Maybe DINIT_RP_SIGNAL_NO_PID. And improve the error message in dinitctl by explaining that the service might not be a process service or it might be in the wrong state.
• sig_num_t type is defined as int32_t in two places. This is wrong, it should be int, and we don't need a typedef for this, just use the right type (int).
• signal number validation in control.cc is not necessary and should be removed. Instead the error value from kill (bpsys::kill) should be checked and if it is EINVAL, then return the BADSIG error reply. The current code uses NSIG which isn't part of POSIX.
• can we prune the supported signal list (in signal_to_int_map) to a sensible set of signals that you might feasibly need to send to a process manually?
• there shouldn't be two versions of base_process_service_test that are doing the same thing, refactor it please.

Edit: some additional:

• remove string_map_size template function, it's not needed - use sizeof instead (sizeof the whole array divided by size of one element gives you the number of elements). Also get rid of string_to_int_map struct and just use a pair. Initialisation of the array is also overly verbose, string_to_int_map("none", 0) could just be {"none", 0}.
• dinitctl man page: sentences should end at a newline

[mobin-2008]

About signal list, I don't understand why we need to trim the list? For better memory use or because many of them have really rare use cases?

When I thought about it, it's reasonable to have a small list of predefined signals. My list include these:

• INT
• HUP
• USR1
• USR2
• KILL
• STOP
• CONT
• TERM

signals which I think reasonable. @davmac314 WDYT?

[davmac314]

When I thought about it, it's reasonable to have a small list of predefined signals. My list include these:

That looks pretty good to me. I would add QUIT and INFO (you need to check if SIGINFO is defined).

[mobin-2008]

Some cases I tested it:

> src/dinitctl signal
dinitctl: signal number/name must be specified
!1> src/dinitctl signal --list
dinitctl: The following signal names are defined for this platform:
dinitctl: none   -> 0   NONE   -> 0   HUP    -> 1   INT    -> 2   
dinitctl: QUIT   -> 3   KILL   -> 9   USR1   -> 10  USR2   -> 12  
dinitctl: TERM   -> 15  CONT   -> 18  STOP   -> 19  
> src/dinitctl signal HUP
dinitctl: service name must be specified
!1> src/dinitctl -p ./sock signal HUP boot
dinit: Service boot terminated due to signal 1
[STOPPD] boot
[  OK  ] boot
> src/dinitctl -p ./sock signal 36 boot # SIGRTMIN+1 on Linux-x86_64
dinit: Service boot terminated due to signal 36
[STOPPD] boot
[  OK  ] boot
> src/dinitctl -p ./sock signal 332109 boot
dinit: Requested signal not in valid signal range.
dinitctl: provided signal was invalid.
!1> src/dinitctl -p ./sock signal junk99 boot
dinitctl: 'junk99' is not a valid signal name/number
!1> src/dinitctl -p ./sock signal 99junk boot
dinitctl: '99junk' is not a valid signal name/number
!1> src/dinitctl -p ./sock signal HUP --list
dinitctl: Invalid command line.
Try 'dinitctl --help' for more information.
!1> src/dinitctl -p ./sock signal --list HUP
dinitctl: Invalid command line.
Try 'dinitctl --help' for more information.

[davmac314]

Some cases I tested it:

Thanks. I think we need to remove "none" and "NONE" from the list of signals displayed.

Have you tried:

dinitctl signal INT boot other

?

[mobin-2008]

Some cases I tested it:

Thanks. I think we need to remove "none" and "NONE" from the list of signals displayed.

Have you tried:

dinitctl signal INT boot other

?

Seems like it tries to find "other". Are we need to fix it?

[davmac314]

Seems like it tries to find "other". Are we need to fix it?

Invalid command lines should cause an error.

[davmac314]

It's getting there, but still some issues. Also need to fix the error cases noted in the PR discussion.

[davmac314]

This is great, thanks!

[mobin-2008]

Closes #134

[james-knippes]

Hi @james-knippes - just wondering if you were still planning on finishing off this PR? I think that tests are the only thing pending

Hey, sorry for taking so long. Got distracted by life stuff. Should have responded, that I did not have the time.
Thank you @mobin-2008 for taking over. Nice work in general and also with the test (:

[davmac314]

@james-knippes no problem, thanks for the work that you did on this.