On Mon, 2010-04-12 at 09:57 +1000, nathans@xxxxxxxxxx wrote:
> ----- "Ken McDonell" <kenj@xxxxxxxxxxxxxxxx> wrote:
> > ...
> > If that goes well, then these should be ready for merging into the
> > main tree, modulo consideration of any review feedback.
> >
>
> Looking good. Couple of minor things...
> - the error messages in __pmLocalPMDA are using an old name for the
> routine (__pmAddLocalPMDA)
Thanks for catching that.
> - seems like do_pmda_spec() should be a libpcp API, ala the other
> parsing routines, since three tools (so far) use it & it is kinda
> like one of the "standard" command line options, almost.
I toyed with this ... I decided not to because libpcp seems to have
become a dumping ground ... PCP started life with a "mean and lean"
charter and once upon a time there were a only handful of routines in
libpcp. Today we're exposing 320+ external functions in libpcp, and of
these 50 are NOT called anywhere in the main pcp source tree (I know
there are proprietary bits still in hiding and the usage from pmchart
and other client tools outside the main pcp source tree, but still!). A
further 55 external routines are only called from one source file.
Now I'm not saying __pmParseLocalPMDA() is not a candidate for libpcp,
I'm just not sure ... although it seems to have a stronger case than
lots of other routines that are included in the library ... 8^(>
> - at this stage, we could probably split libpcp/src/connect.c into
> a pmcd version and a local version, ala fetch.c & fetchlocal.c.
This is more of a religious issue ... I personally don't think code
quality and engineer productivity has much to do with source line count,
but then again I'm just an old fart who hates gratuitious comments in
the source.
> Here's a patch that addresses these. Its untested (ran out of time
> on the train this morning, have to look at other stuff now). The
> parseError() code was pulled from the time window parser, might be
> time to make that shared too (in libpcp).
__pmParseLocalPMDA() eats the whole buffer, so probably does not need
the second argument ... the routine could return NULL for success, else
a pointer to an error message.
Do you want me to refine my changes or are you going to finish your
patch? I think the end result will be much the same, so I'm easy ...
just let me know.
|