initctl signal <name> <signal> for send process signals

[hongkongkiwi]

There are times where I need to send a custom signal directly to a running process.

For example, udhcpc can receive a custom user signal to release or renew an ip if running as a daemon.

It would be great if there was a command in initctl to do this. I am thinking something like:
initctl signal <name> <signal>

For example
initctl signal udhcpc SIGUSR1

I do have a workaround but it is not very elegant:
initctl -b -p status udhcpc| grep '\s*PID\s:' | sed 's|.*: ||' | xargs kill -s SIGUSR1

[troglobit]

Neat idea, I'm using something similar myself right now! :)

[troglobit]

Basic idea for the implementation, @jorgensigvardsson:

1. The user running initctl may not be root so we need the signaling to be done from inside Finit
2. There are already mech set up for stop/start/restart commands, build on top of that
3. The signal command parsing should allow for both SIGTERM, TERM and numeric 15 .. see util.c:signames[] lots of boilerplate is already available.

Note: initctl links against as few .c/.h files as possible, but shares
a few with Finit itself, check src/Makefile.am for the reference.

[jorgensigvardsson]

So, basically, this issue is about locating the correct PID for the currently executing service, and send the requested signal to it.

Shouldn't this be restricted in some way? Normally you can't do this to any other process than your own:

The user running initctl may not be root so we need the signaling to be done from inside Finit

Do you mean that this function is free for all? No group membership required, or finit config specifying which users (or groups) that may do this? Globally or per service (or both)?

[troglobit]

Just to clarify, it's to send a custom signal to a run/task/service managed by Finit. Not unlike the start/stop/restart commands. Maybe that info got lost. (For other processes there's kill(1).)

Yes, it's currently restricted by 1) the execute permission flags on initctl, 2) the socket file in /var/run. By default, Finit installs with root permissions to use any of the initctl commands -- however, a sysadmin or a Linux distribution may choose to have initctl suid root, or change the group permission flags. E.g., all members of the wheel group have permission to manage the system with initctl. Meaning, they are not root and cannot use kill(1) to send signals, but they can use initctl signal. No other mechanisms to restrict access exist at present, and unlike systemd, Finit does not dictate any policy to distributions.

Sorry for the confusion my comment may have caused. I sometimes forget to mention all the specific use-cases, and the details surrounding them, that exist.

[jorgensigvardsson]

Hey no problem! So, if I understand you correctly, the policies are already in place, or at least the assumption is that they are.

I suppose looking at initctl start is a good starting point?

[troglobit]

Yes they are. Locked down by default :)

Yup, they are all the same under the hood. Good luck!

[jorgensigvardsson]

Ok, initctl extended: jorgensigvardsson@bc1bf57

I have extended the command parsing with a new callback cb_multiarg(int argc, char **argv), because I needed two args for the same function. Could not use the same multi level command hack as cond uses.

Also, could not find a precedented way to send multiple arguments to the backend (via do_svc), so I came up with the following convention:

arg1
arg2
...
argN

I just separate the arguments with \n. What do you think?

[troglobit]

Looks very promising so far! 😃👍 I have several comments, but lets start with your question. I think it's better to reuse the data[368] field in struct init_request for multiarg, it has only been used in the other direction (replies from Finit to initctl) so far, but I see no reason why it cannot be used for this purpose, that would also mean we can skip the calloc/free.

However, in this specific case I don't think we need to add multiarg, we only need one extra arg, and for that you can use the runlevel field. It is up to each CMD to define the API. (The field names are only historical, there was an old SysV compatible FIFO in /dev prior to the current UNIX domain socket API.)

Now, for some other feedback:

1. When doing a change like this it's customary to have a separate commit for adding the new API and another commit that uses that API, same custom in many other projects as well
2. Style, I will be very nit-pickety about this, sorry:
   • For one-liners in if-statements there's no need to have braces, you apply this to parts of the patch others not, the if (command[i].cb_multiarg) is particularly interesting since the if statement on the line before does not use braces ...
   • In the usage text Send signal S to service by name, with optional ID, you've added "with optional ID", this is optional in all commands, yet none of the other commands mention this ...
   • Comments, this is not C++, please use C one-liner and block comments, see rest of the code base for examples
3. You need to also update the initctl man page, and the initctl usage example in the README

[jorgensigvardsson]

Yeah, sorry about the brace inconsistency. Combination of typing on a laptop, in a sofa, half watching "Mello", while discussing the merits of the contestants with children. In those situations, all seconds count as the battle with mental fatigue does not afford any complicated movements on the keyboard! 😂 Rest assured, I don't fancy braced single statements.

Comments, I'll fix those, no problems. Old habits die hard!

I'll drop the calloc/free. Embarrassed I didn't snprintf straight into the buffer. Guess I stared too hard on do_svc!

Not sure what you mean about the API part though. Just to clarify, this commit isn't done yet. The intent is to amend and force push it. I had in mind a two part PR: one for initctl, and one for the actual implementation in finit. Is that what you meant in point 1?

In which commit would you like to see the man page changes? I take it goes into the initctl commit, as the man page describes the changed piece of code.

[jorgensigvardsson]

And also, the command description, I'll of course drop the optional ID thing!

[troglobit]

Consider it a pre-review to your PR, glad to you took it so well :)

Yeah, depending on the size of the change of course, but splitting commits in a way that they are easy to review, adds or changes one thing at a time, i.e., bisectable.

The man page update can be a separate commit. Sometimes the README commit can actually be the first commit. Depends on the thing to do, but writing down how someone else should use your thing is a really good design exercise.

Looking forward to your PR, we can do more nit-picking there, if needed 😃

[jorgensigvardsson]

• In the usage text Send signal S to service by name, with optional ID, you've added "with optional ID", this is optional in all commands, yet none of the other commands mention this ...

Now I'm confused. start actually does say with optional IDs. I guess I got it from there. It is a command at the top of the --help output for operations on particular services, so I guess it makes more sense to leave it out on subsequent commands.

[jorgensigvardsson]

There we go, I think I've smacked the commit around a little to suit your taste better: jorgensigvardsson@d3238e4

And I don't mind the nit-picking. I'm basically a contractor doing work in your home, with the promising of me and my coworkers crashing there! 😅

Edit: force pushed a small change that removes a warning about comparing int with size_t

[jorgensigvardsson]

PR coming up real soon. I think I'm doing the right thing in finit, but please let me know if I've missed some needed book keeping. Disclaimer: I have not yet tested it in a running environment. Managed to get 45 minutes of spare time! See the PR as a ground for discussion.

[jorgensigvardsson]

Tried to run it on zero target, but is getting some weird kernel panic:

/sbin/init: error while loading shared libraries: libuev.so.2: cannot open shared object file: No such file or directory
[    0.859587] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00

Not sure why this is an issue. Did specify FINIT_OVERRIDE_SRCDIR=<where I checked out finit>. Also did make distclean in finit source dir, followed by make finit-reconfigure, make and make run. I could clearly see that it had found libuev in the configure output. I also see that the finit package does list libuev as a dependency.

Will have to look at this later this evening. I'm a bit stumped :)

[troglobit]

This looks much better, well done! I'll comment in detail on the PR later tonight; only some minor things.

Re: panic. This is what happens when PID 1 dies, kernel is a slave under PID 1 basically. So we don't do bugs in PID 1 :-)
Check if you have the correct libuev in output/target/lib/ and other dirs, use the file command to verify. Looks like you don't have the correct (or any?) libuEv installed on target, for some reason. Verify also that output/.config contains the BR2_ configs for finit and libuev (and libite).

[jorgensigvardsson]

Thanks for the tip! Will return to computer shortly for a while, and run make with V=1 too. That may also give some clues to what is happening.

[jorgensigvardsson]

Also going to muck around in the output directory. I suppose I will find the target file system in there in a directory used as a basis for the image.

This is an opportunity for learning! Sorry for a low signal to noise ratio on this issue! 😅

[jorgensigvardsson]

Not sure what triggered the panic. I had a look at the target file system under output-zero and it looked good. Perused the buildroot manual and found the section about <package>-dirclean. Did that one, rebuilt finit, and ran the image. Works!

Now I have other issues. finit doesn't seem to send the request (found initctl debug 😄 )

[jorgensigvardsson]

Found the issue. Looks like init_request structures need to be fully reset after one usage. Also, it turns out that make <package>-dirclean is sometimes a must to have a redeployed application. Not sure why. I could've sworn I got some changes deployed on the target image, and sometimes it did not.

[jorgensigvardsson]

I have now successfully killed mdnsd using the command initctl signal mdnsd kill. The daemon died, but finit resuscitated it (as expected). Also sent HUP to it, with the debug flag toggled in finit, and I could see logs referencing config file reloading. Do you know any better way to validated the feature? (Building basis right now to validate on it as well).

Did some git sorcery, updated the history (rebased bug fix onto first commit, as the fix was for that particular commit, keeping the noise low in the PR).

[jorgensigvardsson]

Basis build validates the signal feature of finit.

[troglobit]

Rebuilding in Buildroot sometimes requires -rebuild, sometimes, -reconfigure, and sometimes a -dirclean is needed. It depends on what you do. The manual explains this in a bit more detail :)

Great work so far!

HUP and KILL we already sort of test, op suggested this feature to send USR1/USR2 to udhcpc. I guress this is as good a time as any to create a test case. You can add signal support to the shell script used, server.sh and use it to trigger some behavior your test checks for. See my latest start-kill-service.sh for an example. (Note, you need to run configure --prefix=... etc. properly, or the unit tests will fail spectacularly -- see test/README.md, which I just updated.)

[jorgensigvardsson]

Nice! Not at the computer right now, but I presume these scripts all use unshare?

[troglobit]

Yes, it's actually a framework designed by Jacques, so if you run into any issues with the framework he might be better to ask :)

[jorgensigvardsson]

I think I have an issue with finit and the saving of random seeds. Maybe I'm missing something in my environment? Tests checkself.sh and setup-root.sh all work fine, but after that I get all fails. When running them manually, I see problems with storing random seeds:

It looks like the test pass, but something is going wrong in finit. Maybe I have a kernel configuration that finit doesn't like? 🤷

I'll look into it later. Have to get kids to school now, and then $DAYJOB starts! :D

[jorgensigvardsson]

I tracked the error down to this:

print_result(RANDOM_BYTES != copyfile("/dev/urandom", RANDOMSEED, RANDOM_BYTES, 0));

When looking in tenv-root/var/lib/misc, I see:

drwxr-xr-x 2 jorsi jorsi 4096 feb 14 07:44 .
drwxr-xr-x 5 jorsi jorsi 4096 feb 14 07:44 ..
-rw------- 1 jorsi jorsi  512 feb 14 07:45 random-seed

The code specifies that 32 KB should be saved, but only 512 bytes are saved. Have no idea what's going on here, but my suspicion is that there is not enough entropy to save 32 KB. Will try testing without random seed turned on.

[troglobit]

@jorgensigvardsson Let's move that topic to a separate discussion/issue, OK? We need to at least try to keep this one on point.