Hi Nathan,
On 08/27/2013 03:26 AM, Nathan Scott wrote:
1. QA
Some modified QA tests will now fail for when run against non-latest
sources, as they unconditionally expect "local:" in the output. Ken's
expressed a need to run new QA with older sources in the past, we need
to make this work (anyone doing QA testing gets instant priority over
anyone else in my book ;) ... except maybe people working on security
issues, and then only grudgingly)
I don't think this is tricky though; I'd recommend keeping the output
as local: in the cases where we've now got "localhost" -> "local:" in
the output, and in the tests translate forward (sed: s/.../local:/g)
I agree with Ken's philosophy and this would be an oversight on my part.
I'll handle it as you have suggested.
2. Hostnames
So, this is more of a concern to me - we've talked about this before,
and I remain worried. :) We now have a number of tools that would've
printed the hostname (via the now removed gethostname calls), instead
will print "local:". Some IRC discussion happened about this one and
I'm pretty sure its going to bite people if we don't tackle it.
So, looking about in libpcp, I discovered pmGetContextHostName today!
I don't remember ever seeing this before but Happy Days!, it means we
don't have to add to the PMAPI to solve our problems! (We do need to
make many more tools use it though). We should add a "pmcd.hostname"
metric pmFetch() in there (like pmlogger does now) and start to use
the result. For hosts without that metric, and which *are* local (ie
where "sts = ctxp->c_pmcd->pc_hosts[0].name" would give a path or the
"localhost" string), we should fallback to the gethostname route.
This way we solve my both concerns about hostnames and Franks cunning
plan to get more accurate hostname reporting in the clients (ie. over
ssh tunnelled connections and so on). Win, win!
I've review the code that you reference here and it makes sense to me.
Stay tuned for an update.
In other discoveries resulting from libpcp spelunking today, I think
__pmSetClientId is broken wrt IPV6, right? Some of the comments do
not look too promising, and its not using new network APIs.
Yeah. It ignores non-inet addresses completely. Should be easy to bring
it up to speed.
3. Possible buglet?
Noticed this pattern... (auxconnect.c, secureconnect.c)
else if (addr->sockaddr.raw.sa_family == AF_UNIX) {
/* Simply truncate the path in the address to the length of the mask.
*/
i = strlen(mask->sockaddr.local.sun_path);
addr->sockaddr.local.sun_path[i] = '\0';
... are we guaranteed that the mask string is longer than the string
we're truncating? Else, that looks like a buffer overrun.
This is ok. The sun_path is a fixed length buffer and the length is the
same for all addresses. It is guaranteed to be nul terminated. If the
mask is longer than the string we're truncating, then this function has
no effect (as desired).
4. Cleanup
I think the new impl.h routine you've added for pmcd to do some
extra AF_UNIX setup might not be needed - can we do that work in
__pmServerAddNewClients? I think so...? It already has a little
bit of AF_UNIX-specific skullduggery, and the NewClient callback
happens almost immediately after the point you've changed within
pmcd (and, importantly, before the access checks - they're actually
done in the code that pmcd plugs into that NewClient callback).
The call chain I see is __pmServerAddNewClients -> CheckNewClient ->
AcceptNewClient -> (__pmAccept + __pmServerGetLocalSocket)
The problem I see is that the address path must be set after __pmAccept
(that's when we find out it's AF_UNIX) and by the time we return to
__pmServerAddNewClients, it's too late (do we even have access to the
client's address from within __pmServerAddNewClients?). The access check
(__pmAccAddClient) is done by CheckNewClient immediately after
AcceptNewClient.
The only place I see to do this within libpcp would be within __pmAccept
itself, which would be nice, since this code is intended to fill in the
address for AF_UNIX clients (accept(3) and PR_Accept() don't do this for
some reason although, by design, they do it for other address families).
However, then there would be an assumption within __pmAccept that only
one unix domain socket client exists for each server, which may or may
not be a problem.
Dave
|