Hey Dave,
----- Original Message -----
> Hi,
>
> I've been looking into the ins and outs of adding the capability of
> listening on unix domain sockets to pmcd (and presumably pmproxy, and
> perhaps other components) via extensions to ...
As discussed (off-list I think? can't seem to find a reference now
anyway) I took a pass over latest-and-greatest from the brolley/dev
branch over in sourceware.org/pcpfans. Loo-king good! Here's every
little persnickety note I made while reviewing ...
QA
- Some QA tests now getting up to 10 alternate output forms
(maintenance of alternate competing version becoming a problem)
- for the tests which are exercising access control on remote
hosts, theres no reason (AFAICT) that we need to make sure
the unix domain socket log lines are in the output (there's
nothing being tested there)
- thats not the case for IPv6 so while we did need to add the
conditional output for that, IMO we don't need to add it in
the case of unix sockets.
- Thoughts? (so, I think we should filter out the unix-domain
lines and stick with the existing .out files for most/all of
these tests).
auxconnect.c
- __pmInitSocket
- API change - OK, cos its an internal.h routine only?
- need the SO_LINGER on unix domain sockets? if not, we could
short-circuit the routine and have just the one #ifdef ...
#if defined(SOCKADDR_UN)
if (family == AF_UNIX)
return fd;
#endif
... and one less syscall. *shrug*
- __pmPMCDLocalSocketDefault()
- no need for the '= ""' in pmcd_socket, statics init'd to zero
- snprintf needs to pass sizeof(pmcd_socket) - 1 to ensure null
termination (two spots)
- inconsistent comment style with surrounding code
- __pmCreateUnixSocket()
- inconsistent comment style with surrounding code
auxserver.c
- localSocket{Path,Fd} don't need to be conditional AFAICT (?)
(simplifies other code later - less #ifdef sprawl, more readable).
- OpenRequestSocket()
- we make an immediate decision based on value *family but this
is not guaranteed to be initialised -> see 3rd callsite in
OpenRequestPorts()
- Dave-style comments, inconsistent with surrounding code
- __pmServerCloseRequestPorts()
- new code doesn't need to be #ifdef'd
- no need for assignment to "i" in new code?
- second diagnostic should probably also report gid/egid given
expected filesystem permissions on PCP_RUN_DIR?
- make into a single call to __pmNotifyErr, double-equals isn't
really the common convention - just use single-equals I reckon.
- __pmServerAddNewClients
- new code here doesn't need to be #ifdef'd
- __pmServerDumpRequestPorts
- __pmServerRequestPortString
- new code in both doesn't need to be #ifdef'd
check-statics
- if we don't remove the conditional check on new statics above, build
will fail in this script I think (wont be able to find global static
variables that we're now adding).
config.c
- release number in comment is off-by-one now cos I snuck a release in
last week
connect.c
- pmcd_socket can be declared unconditionally? check-statics will want
that IIRC.
- can avoid yet another global by merging new load_pmcd_local_socket()
into load_pmcd_ports (rename to load_pmcd_socket_config or something
along those lines?)
- __pmConnectPMCD()
- I *think* for the case where we set "sts = -2" in new code, we'll
fall into a bad state in the first diagnostic after "if (sts == -1)"
(which does the usual inet/ipv6 socket connections)? at very least,
wont it print "... port= failed: ..."? (ie miss printing the port)
secureconnect.c
- some Dave-style comments
- __pmSetSockOpt - there's no SO_PASSCRED on Win32/Win64 - sneak it into
the existing #ifdef there for SO_EXCLUSIVEADDRUSE perhaps?
spec.c
- need a new QA test along the lines of test 720 which invokes the host
parser with unix socket addresses with valgrind, checks and dumps 'em
- doesn't feel right to have the parser logic have #ifdef conditional goo
all over it like this - elsewhere in pmNewContext we should be dealing
with lack of support (ie lower down) anyway, so we should not have to
do it here too. I think. Right? Simplifies life here - no need for
the additional parseProtocolSpec argument, just parse it & pass it on.
- also, use PCP_ATTR_UNIXSOCK for unix: prefix, add in a PCP_ATTR_LOCAL,
and return *attribute from parseProtocolSpec. Then, you wont need to
do the check-to-see-if-value-non-null-then-strcmp-it dance back in the
caller - just use the return code. You also wont need the new isLocal
and isUnix variables in the caller, so code should become simpler.
- a sprinkling of Dave-style comments
pmcd.c
- sockpath does not need to be conditional
- so - just unconditionally accept the command line arg (-s)
- then, either make __pmServerSetLocalSocket return an error code if not
supported, or better still make __pmServerOpenRequestPorts report and
fail later on.
- that new __pmPMCDLocalSocketDefault call here feels wrong too - could
this not be hidden away inside __pmServerSetLocalSocket, removing need
for pmcd and other servers to have to deal with setting defaults? Use
__pmProgname in there, if it needs different xxx.socket name for pmcd/
pmproxy/pmwebd? One less symbol/function in libpcp too then, and one
less interface exposed by impl.h that we need to keep back-compatible.
cheers.
--
Nathan
|