Code changes for this review have been pushed to the brolley/dev branch
of the pcpfans repository. qa testing is underway.
Comments below ...
On 06/24/2013 06:04 AM, Nathan Scott wrote:
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).
I do agree that the management of possible outputs is be coming
cumbersome, especially when the closure of multiple configuration
elements is at play. There are no specific tests for the use of unix
domain sockets (yet), so the unix domain line in the pmcd request ports
table is now filtered in the existing tests, as suggested.
auxconnect.c
- __pmInitSocket
- API change - OK, cos its an internal.h routine only?
Hope that's ok.
- 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*
My understanding of SO_LINGER doesn't make it more or less applicable to
a unix domain socket as opposed to a tcp one. As a compromise, I changed
the code to set the SO_LINGER option first allowing your suggested
short-circuit of the rest of the function.
- __pmPMCDLocalSocketDefault()
- no need for the '= ""' in pmcd_socket, statics init'd to zero
ok
- snprintf needs to pass sizeof(pmcd_socket) - 1 to ensure null
termination (two spots)
Not according to my reading of the man page. It says that at most 'size'
bytes will be written which includes the terminating nul and that the
terminating nul is always written unless 'size' is zero. These are the
reasons why I always use snprintf() as opposed to strncpy() which only
writes the terminating nul if there is room.
- inconsistent comment style with surrounding code
- __pmCreateUnixSocket()
- inconsistent comment style with surrounding code
ok
auxserver.c
- localSocket{Path,Fd} don't need to be conditional AFAICT (?)
(simplifies other code later - less #ifdef sprawl, more readable).
OK. I didn't know what the policy was for unused code, in the sense that
these statics will never be modified and the tests involving them will
always go the same way on platforms on which unix domain sockets are not
supported. Some projects might prefer that these objects and the
associated code not exist in those builds. I actually prefer it the way
you want, so consider this done.
- OpenRequestSocket()
- we make an immediate decision based on value *family but this
is not guaranteed to be initialised -> see 3rd callsite in
OpenRequestPorts()
Good catch! *family is now initialized to AF_UNSPEC at the 3rd call site
making it now initialized at all call sites. I've added a
(non-Dave-style) comment to this effect in the function header comment.
- 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?
ok
- second diagnostic should probably also report gid/egid given
expected filesystem permissions on PCP_RUN_DIR?
Actually the second diagnostics was something I added in order to help
me debug the problem of not being unable to unlink the socket path in
the first place. I had intended it to be temporary. If you think it
would be useful, then yes, the additional info should also be reported.
- 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
OK. Done
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).
Well, I had them prefixed with '?' which makes them optional, but that's
a non-issue now.
config.c
- release number in comment is off-by-one now cos I snuck a release in
last week
ok
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?)
Actually, it turns out that these are all unnecessary remnants from a
time before __pmPMCDLocalSocketDefault(). They have been removed.
- __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)
This all looks ok to me. -2 is intended to indicate "no more connection
attempts" after failing to connect to the unix domain socket (i.e.
unix:[//path] was specified). In this case, we want to bypass the normal
tcp connection attempt(s) and go straight to the diagnostic in question.
Not printing the port(s) is intentional, since unix domain sockets do
not have ports.
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?
OK
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
Coming soon to a test suite near you.
- 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
Much better, thanks. I had thought that PCP_ATTR_PROTOCOL was for all of
the prefixes.
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.
Done, done, done and ..... done
Thanks again for the review,
Dave
|