pcp
[Top] [All Lists]

Re: [pcp] RFC: libpcp hash table iterators

To: "Frank Ch. Eigler" <fche@xxxxxxxxxx>
Subject: Re: [pcp] RFC: libpcp hash table iterators
From: Nathan Scott <nathans@xxxxxxxxxx>
Date: Fri, 8 Mar 2013 15:19:35 -0500 (EST)
Cc: pcp@xxxxxxxxxxx
Delivered-to: pcp@xxxxxxxxxxx
In-reply-to: <20130308164357.GC4559@xxxxxxxxxx>
Reply-to: Nathan Scott <nathans@xxxxxxxxxx>
G'day!

----- Original Message -----
> Hi -
> 
> Please consider pulling the sole patch in pcpfans.git fche/hashiter,
> as preparation for the pmwebapi work.  This little iteration function
> is the only thing needed in the core for pmwebapi, which should
> simplify its testing between 3.7 and 3.8.

I'm hesitating a bit, cos it's late in the release.  While there's no
in-tree user atm, its clear pmwebapi will use it so I think its OK on
that front.  The other hash functions in that file are tested via their
extensive use in the rest of PCP but this code is untested, so that's
less good - a little qa test would help.  :)

On review, can I also suggest:
- call it __pmHashWalk  (consistent with pmdaCache interface names)
- whitespace is inconsistent with rest of that source file
- there are no users even in pmwebapi of the PM_PHI_STOP and
  PM_PHI_STOP_DELETE codes.  I suspect they may never be used...?
  (could add em later if needed, I'd stick with what we know we need)
- pmHashIterFn_t -> __pmHashWalkCallback (underscore-t style not used
  in PCP, double-underscore cos its internal, "callback" name is used
  in a few other places).
- so, if "walk" convention agreeable, perhaps go with similar macros
  PM_HASH_WALK_NEXT (for PM_PHI_CONTINUE),   (like PMLOGREAD_NEXT, 
PMDA_CACHE_WALK_NEXT)
  PM_HASH_WALK_DELETE (for PM_PHI_CONTINUE_DELETE).

Also worth thinking about, the pmda caches avoid malloc/free and opt
instead for active/inactive nodes - I'm not sure whether that might
be appropriate for these hash entries (haven't gone through pmwebapi
in detail yet), but further food for thought.

cheers.

--
Nathan

<Prev in Thread] Current Thread [Next in Thread>