>>> On 8/22/2008 at 10:27 AM, in message <[EMAIL PROTECTED]>, Carlo Marcelo Arenas Belon <[EMAIL PROTECTED]> wrote: > On Thu, Aug 21, 2008 at 02:57:02PM -0600, Brad Nicholes wrote: >> >>> On 8/21/2008 at 12:19 PM, in message >> <[EMAIL PROTECTED]>, >> <[EMAIL PROTECTED]> wrote: >> > http://bugzilla.ganglia.info/cgi-bin/bugzilla/show_bug.cgi?id=200 >> > >> > [EMAIL PROTECTED] changed: >> > >> > What |Removed |Added >> > ---------------------------------------------------------------------------- >> > CC| |[EMAIL PROTECTED] >> > AssignedTo|ganglia- |[EMAIL PROTECTED] >> > |[EMAIL PROTECTED]| >> > |et | >> >> > >> > ------- Additional Comments From [EMAIL PROTECTED] 2008-08-21 11:19 >> > ------- >> > there was a reason why libgmond values were set to NULL (maybe so it can be >> > written over if needed, and easily detected to be an empty value, but >> > can't > >> > recall). >> > >> > there is a note in gmond.conf which is related : >> > >> > B<NOTE>: Currently gmetad cannot process data sources that do not >> > have a cluster tag. This will be remedied in the future. >> >> Carlo, > > Brad, > >> So what do you want to do with the patch? > > short version: punt for next release and tag 3.1.1 without it > longer version below >
OK >> I don't think that specifying a cluster default name is going to hurt >> anything since having no cluster name is a misconfiguration. Also the >> default gmond.conf file provides a default anyway for all of the cluster >> values. > > true, but this patch is also changing the way this structure is allocated > from > heap to some data segment closer to the code and which needs to be treated > more carefully (as it will be a perfect target for a security point of view) > I'm not sure what you mean here. This patch just sets a default value for libconfuse just like other defaulted configuration directives. > and this patch is changing libganglia and therefore, at least by definition > affecting more than just gmond. > true. >> After looking at the XML parsing code, I can see why the above note was >> necessary. The parser can't handle a host without a cluster. > > presume you mean the gmetad XML parsing code here, and yes I would expect > that to be the case (a cursory look shows it is probably enforced by our > DTD) > but then some other consumers from gmond data might not have that > requirement > at all. > > haven't had time yet to look at the impact of the change in the rest of the > code to do any review, but as I mentioned in that comment, the fact that > someone decided long ago to put a comment in the configuration instead of > fixing it the obvious way (which should be something like your patch), > probably means that there was a reason why it wasn't a good idea (at least > at that time), and then again cursory looking of the code shows that those > attributes are being "free" and reassigned to point to heap allocated > objects > and therefore in some platforms they could fail and are of undefined > behaviour. I don't believe that this patch would affect any gmond code at all. Gmond simply asks for the cluster section values from libconfuse. Gmond has no idea whether the returned values came from an actual configuration file value or a defaulted value. The only difference once the patch is applied is that libconfuse will never return a NULL value which was the root cause of the segfault in the first place. Brad ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
