Thanks for the review Nathan. Your comments are spot on. Some responses
below.
Dave
On 08/23/2012 11:39 PM, Nathan Scott wrote:
Hi Dave,
I've managed to lose your last mail on this topic, so these
notes are just free-form as I read through the code in your
current pcpfans nss branch.
Overall its looking like its progressing along nicely. I'd
like to start getting some of the "wrapper" style changes
(API level) merged soonish (pcp-3.6.7), as they stand alone
and can go in anytime. This might help to break up the work
into smaller chunks, rather than us having a single whopping
big commit arrive down the track. I'll start trying to help
out on that next week.
Thanks. Yes. I feel like this branch is getting old and crusty. I would
like to run the entire testsuite against the current dev branch and then
with my changes for NSS/NSPR enabled and disabled, all with the same
results before we push it.
== NSS/NSPR notes
- configure.in: explicit require on fedora/redhat... hmmm?
this would mean cannot monitor a RHEL server from, say, a
Mac OS X pmchart? are their known issues with the other
platforms that require this check?
Any requirement placed on RHEL or Fedora is purely temporary. I'm just
not expert enough to know whether the particular test framework that
used (stolen from systemtap) is portable. Any assistance is appreciated!
- LogImport/Makefile.PL ... this change seems unnecessary?
(logimport is all about archives, not live monitoring)
Ohhh... this is part of a bigger linkage issue:
Will all out-of-tree pcp client tools will have unresolved
NSS symbols on the next pcp upgrade? ... that would be bad.
Isn't there a way to link one shared library with another?
Ideally, there is and we'd link libpcp that way. Otherwise
this is going to be very intrusive for the punters.
Agreed and, once again, not intended. Also, once again, any assistance
is appreciated.
- pmio.h -> impl.h &| platform_defs.h? (prefer no new header)
OK. I'll get rid of pmio.h
- auxconnect.c
-> hmm, this seems to create a second file descriptor
table? I was expecting this code to all be over
in ipc.c in libpcp, and augmenting the existing
table there rather than dup'ing it. Means we're
doubling up on memory allocation calls, PM_LOCK/
PM_UNLOCK calls, etc for every socket we create?
-> checking back on Kens mail, yeah - he said the
same thing... was there a reason this approach
didn't pan out?
| What I had in mind was something along these lines:
|
| * change the __pmIPC structure as follows:
| * change socket to be flags and define bit field flags to
| encode the channel's type (socket or otherwise for
| Windows), clear text or encrypted (for your use), etc.
| * add an int fd field for the clear text case
| * add whatever extra "stuff" you need to make encrypted
| I/O work
Oh well, I had asked whether he was thinking about putting it in the IPC
table or a separate table and his response was along the lines of what
you've outlined above. I decided not to play with such a vital table
right away. Now that the basic work is done, it does seem like it would
be easy to integrate.
- __pmSelectRead? not __pmSelect? (shrug, just stood out)
Yeah. I noticed that nowhere in pcp does both at once and the
abstraction worked out pretty easily if one or the other was assumed. A
general implementation could still be done if you like.
- linux/pmda.c - ugh! in hind-sight, surprising we've not
come across this before. Might be something we should be
doing better in PCP (I'll have a look, see if this becomes
a big change, if not will rename domain definitions to be
more unique - PCP_LINUX_DOMAIN, for example)
Seems like a NSS/NSPR namespace violation to me, but yes.....ugh!
Will talk more next week - I'm away for the weekend shortly,
and wont be checking mail for a coupla days.
OK. Hope you had a great weekend!
Dave
cheers.
--
Nathan
|