[Cluster-devel] Re: Groupd uevent clean up

Steven Whitehouse swhiteho at redhat.com
Tue Nov 25 16:44:32 UTC 2008


Hi,

On Tue, 2008-11-25 at 09:47 -0600, David Teigland wrote:
> On Tue, Nov 25, 2008 at 02:08:42PM +0000, Steven Whitehouse wrote:
> > 
> > The following patch is designed to clean up a number of items relating to
> > gfs_controld's handling of uevent notifications. When a uevent is received
> > is consists of a number of strings whose total length is bounded by the
> > buffer size and returned by the recv call.
> > 
> > Originally only the initial string was being used to select the various
> > actions. That was wrong for various reasons, but mostly because its much
> > easier to simply look for the individual fields, broken out from the
> > message. The other reason that this is wrong, and also the reason that
> > I started looking into this area initially is that the initial event
> > string provides a "physical" view of the event. By using the SUBSYSTEM=
> > field, we get the same information, but it can also be overridden by
> > the kernel using the kset_uevent_ops which will become important when
> > we merge the lock_dlm module into GFS2.
> > 
> > The other problem (unsolved here) is that we have to parse the fs name
> > from the DEVPATH= variable. Its trivial to add a feature to the kernel
> > to provide the LOCKTABLE as well, so I plan to do that in the future
> > so we don't have to jump through hoops to get that information.
> > 
> > At the same time I've marked a bunch of variables const. Mainly
> > because they started to throw up warnings after I'd added the
> > new event parsing code.
> > 
> > I've tested the parsing code on its own, but since I don't have the
> > right packages installed here, I've not had a chance to build &
> > test a complete new groupd yet.
> 
> OK, thanks, I dislike the const sprinkling, and I don't care about string
Whats wrong with adding the consts? It seems to make sense to mark the
fsname as const since its just being used as a key for looking up the
mg.

> parsing.  I'll take your word that this string parsing is an improvement
> over what's there (I really dislike string parsing, previously get_args()
> was used for splitting args from other strings, too).  All I know is that
> what's there works, so if you can verify that the new parsing really works
> I'll add it (or I can do a quick test if you resend a patch with just that
> bit).
> 
The reason for changing the parsing is really that it was looking at the
wrong string from the set of strings that uevent sends. It should be
trivial to find the right bits, but for some reason, we didn't add the
locktable to those variables. I hope to add that in the future, so that
we might in time be able to avoid messing about chopping bits out of the
DEVPATH= string in order to find the locktable.

My dev machine currently has a strange mix of F-9 and (old) rawhide
packages on it, so that installing the required bits of openais to build
isn't so easy. If you are able to run a quick test, that would be
helpful otherwise, I'll look for another place to build it for testing.

> > Also, I've fixed up some broken error handling in process_uevent
> > at the same time.
> 
> Yep, good catch, I'll add that small bit right now.
> 
I've just noticed dlm_controld has this same bug,

Steve.





More information about the Cluster-devel mailing list