On Fri, 2018-10-05 at 12:10 -0500, Benjamin Marzinski wrote: > On Fri, Oct 05, 2018 at 12:25:15PM +0200, Martin Wilck wrote: > > On Thu, 2018-10-04 at 11:45 -0500, Benjamin Marzinski wrote: > > > On Mon, Oct 01, 2018 at 10:09:41PM +0200, Martin Wilck wrote: > > > > On Fri, 2018-09-21 at 18:05 -0500, Benjamin Marzinski wrote: > > > > > tur_devt() locks ct->lock. However, it is ocassionally called > > > > > while > > > > > ct->lock is already locked. In reality, there is no reason > > > > > why we > > > > > need > > > > > to lock all the accesses to ct->devt. The tur checker only > > > > > needs > > > > > to > > > > > write to this variable one time, when it first gets the file > > > > > descripter > > > > > that it is checking. It also never uses ct->devt directly. > > > > > Instead, > > > > > it > > > > > always graps the major and minor, and turns them into a > > > > > string. > > > > > This > > > > > patch changes ct->devt into that string, and sets it in > > > > > libcheck_init() > > > > > when it is first initializing the checker context. After > > > > > that, > > > > > ct- > > > > > > devt > > > > > > > > > > is only ever read. > > > > > > > > I like the lock removal a lot, but not so much the conversion > > > > into > > > > a > > > > string. Why not keep the dev_t? > > > > > > Because we will simply convert it into a string every time we use > > > it, > > > instead of doing the work one time. It's 24 more bytes in the > > > tur_checker_context, but the code is easier to read, and we're > > > not > > > doing > > > the same work again and again. > > > > Well, IMO snprintf(buf, N, "%d:%d", major(x), minor(x)) is not > > _that_ > > much work, and it looks a lot cleaner, to me at least. > > > > But OK, I'm not insisting on this. So, if you re-post, I'm going to > > ack > > the patch. > > > > My thought for long term is is this actually this: ct->dev_t is > > only > > needed for creating messages to be stored in the checker->message > > string, which I don't like either. > > > > Why don't we replace the "message" field with an "int msgid", and > > add a > > > > char* (*get_message)(int msgid) > > > > to the checker API? > > I would not have a problem with a patch that did this. Let's get your series finalized, and possibly merged, first. Martin -- Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
