Hi,
Nathan Scott <nathans@xxxxxxxxxx> writes:
[...]
> This is looking pretty good - I've updated some minor stuff (please do
> a review) but otherwise its all merged now. There's a few things that
> would be good to still get in that I didn't tackle...
Thanks for merging this. I've worked on the comments below.
> - I'm not following why the (static) metrictab has all those entries
> for event counters still? Can those be removed now?
Yes it can, fixed on lberk/papi.
> - The permissions_check() changed to only look for uid==0 - should we
> drop all references to gid* elsewhere? I think so (slims down some
> data structures, simplifies the attributes callback, etc)
I've done this as well on lberk/papi.
> - As per earlier mail, there's no error handling for auto-fu counters.
> Simplest will be, I think, to give refresh_metric() a parameter that
> indicates whether its caller is going to ignore the return code, and
> in that case, handle_papi_error() should really log any errors into
> the pmdapapi.log file. In the pmStore-enable/disable-metrics case,
> these need not be logged (the client tool will get notified about an
> error in that case, which is better - keep that pmDebug diagnostic
> for both cases though).
I believe the patch that Frank proposed for this has been merged
upstream. The rest of the issues have been addressed in my latest pcp
updates mail[1].
Thanks for reviewing and merging my changes!
Cheers,
Lukas
[1] - http://www.pcp.io/pipermail/pcp/2014-November/006012.html
|