modify rpc.tlsservd so that it can optionally run multiple daemons

[rmacklem]

Summary

During discussions with someone that was doing NFS-over-TLS
development for Solaris, we had a concern that the server might
become overloaded after rebooting, due to a large number of
TLS handshake requests from clients.

To alleviate this potential problem, this patch modifies rpc.tlsservd
so that it supports the "-N/--numdaemons" command line option,
which specifies that up to RPCTLS_SRV_MAXNPROCS (currently defined
as 16 in the patch) may be started.

When there are multiple daemons, one is selected by the patched kernel
in a round-robin fashion, to serve a TLS handshake request.

in order to share the load,

Test Plan

Has been tested with a modified FreeBSD kernel for both
the default case of 1 daemon and with multiple daemons.

I am not capable of testing a heavy client NFS-over-TLS
load.

[rmacklem]

Oh, and I forgot to mention that I will do the
man page update as a separate review/commit.

[emaste]

usr.sbin/rpc.tlsservd/rpc.tlsservd.c

193–194

perhaps asprintf()? it takes care of the malloc for you

224–225

IMO line splitting after the || makes it more clear (even if style(9) suggests the current form)

if (rpctls_procs < 1 || 
    rpctls_procs > RPCTLS_SRV_MAXNPROCS)

[rmacklem]

Includes the two minor changes suggested by emaste@.

[rmacklem]

Hi Mike,

I thought you might be willing to take a look
at this one. Ed hasn't been able to check the
use of the signals when daemonizing, which
is the main area I'm wondering about?

[• karels]

I skimmed it now, will review more carefully tomorrow.

[• karels]

Two questions: (1) Why did you change the shutdown signal from SIGTERM to SIGUSR1? And (2) why do a shutdown on a SIGCHLD? That would mean that killing one of the workers would make all the instances go. away.

[rmacklem]

I'll answer #2 first.
The kernel assigns TCP connections to the daemons
in a round-robin fashion. Therefore, when one server
daemon has failed, some mounts will work and others
will not.

My feeling was that this would be confusing for sysadmins
and it would be better to have the daemons all go away,
which the sysadmin can easily see via ps(1).

For #1, I can only say "tradition". The code was cloned
from nfsuserd.c and then modified. It seems to have inherited using
SIGUSR1 from nfsd.c.
Why?
I can't think of a good reason for using SIGUSR1 instead
of SIGTERM, but nfsd.c has been doing so for, I would
guess, decades.

If you think using SIGTERM would be better, I can try
modifying the code and, assuming it works, replace
the patch here. What do you think?

[rmacklem]

This version of the patch is fixed so that SIGCHLD does
actually terminate all the daemons. (The unblocking of
SIGCHLD was missing in the previous version.)

I also added handling of SIGCHLD being posted to a
child/server daemon. This should never happen, but
I thought it was safer to add code to handle this case.

[• karels]

Sorry, I had lost track of this review.

I don't see the definition of RPCTLS_SRV_MAXNPROCS. Is there a header missing, or is it elsewhere?

I don't see any problem using SIGTERM, just replacing all instances of SIGUSR1 with SIGTERM and removing the current SIG_IGN of SIGTERM. That would seem more conventional and expected. Then "kill $master" would work as expected. Granted, "service tlssrvd stop" is more correct. What do you think?

Otherwise seems OK. I'm a little nervous that the wait call doesn't check for errors, but I can't think of a scenario where it would actually be a problem.

[rmacklem]

Change the termination signal to SIGTERM, as suggested
by karels@.

RPCTLS_SRV_MAXNPROCS is defined in sys/rpc/rpcsec_tls.h,
which gets installed in /usr/include/rpc/rpcsec_tls.h.
(The kernel changes are already in main, done by a previous
commit.)

Thanks for looking at it, Mike.

[• karels]

Thanks for switching to SIGTERM, I like this better.