On 07/07/2014 08:48 PM, Frank Ch. Eigler wrote:
Hi -
The pcpfans.git fche/pmwebd contains a reviewworthy state of the
pmwebd+graphite code. It's just been rebased based on /dev, and
contains a bit of a mishmash of commits (I could squash them into one
if y'all so desire). All the new code is under src/pmwebapi, which
was reworked into C++ and much enhanced. Man pages have been updated.
The /etc/pcp/pmwebd/pmwebd.options file has a more standard & more
useful format. The qa/660 testsuite has been extended based upon a
gcov (test/code-coverage) analysis of the source code, so that
apprx. only trivial or hard-to-trigger error cases are left untouched.
I finally made time for my much-promised review of this work. I first
familiarized myself with the existing src/pmwebapi code on the current
dev branch. With that as a basis for review I looked at the new code on
your branch. What I see there is a translation of the old to code to C++
plus the addition of support for graphite.
The addition of the graphite support is a straight-forward extension of
the existing pmwebapi support and there is really nothing to comment on
there. It looks like it was done in a way consistent with the existing
support.
So, I turned my attention toward the C++ re-implementation, looking for
potential problems in the translation, resource leaks and the like.
Despite my best efforts, I was only able to identify one potential
problem in the function pmwebapi_gc (). The loop relies on iterators of
a map remaining valid while deleting elements from the map. However,
even this turns out not to be a problem based on my reading of
http://www.cplusplus.com/reference/map/map/erase/ (see Iterator Validity).
So, congratulations! I am unable to find any issues with this work!
Dave
|