Re: [PATCH] Keys: Add possessor permissions to keys

[Andrew Morton]

David Howells <dhowells@xxxxxxxxxx> wrote:
>
> Andrew Morton <akpm@xxxxxxxx> wrote:
> 
> > The above bit needs to be captured in a code comment.  Because:
> 
> Okay.
> 
> > Is hair-raising and makes people want to come after you with a stick ;)
> 
> If people get upset by this sort of thing, they shouldn't be doing kernel
> development.

hrmph.  Of course it's a reasonable trick from a performance and
convenience and resource consumption POV.  But it's a new idiom and the
threshold for new idioms is non-zero.  We use it in struct page, but struct
page is special.

It does need really obvious commenting.  Pity the poor person who spends
ten miniutes trying to find the definition of struct key_ref.

> ...
> > > +			if (PTR_ERR(key_ref) != -EAGAIN) {
> > > +				if (IS_ERR(key_ref))
> > > +					key = key_deref(key_ref);
> > > +				else
> > > +					key = ERR_PTR(PTR_ERR(key_ref));
> > > +				break;
> > > +			}
> > > +		}
> > 
> > That's getting a bit intimate with how IS_ERR and PTR_ERR are implemented
> > but I guess we're unlikely to change that.
> 
> You're referring to the ordering of the first two lines? I could, and probably
> should, reorder them.

Yup.  Logically we shouldn't use PTR_ERR unless IS_ERR is known to be true.
Yes, it works and yes, it'll surely continue to work.  But.

> It's also wrong: there should be a ! before the IS_ERR.
> 
> I've changed this to:
> 
> 			if (!IS_ERR(key_ref)) {
> 				key = key_deref(key_ref);
> 				break;
> 			}
> 
> 			if (PTR_ERR(key_ref) != -EAGAIN) {
> 				key = ERR_PTR(PTR_ERR(key_ref));
> 				break;
> 			}

OK.

> > This all seems quite inappropriate to -rc2?
> 
> Which -rc2? If it's 2.6.14-rc2 you're referring to, then yes - that's already
> released.

It doesn't fix any bugs (does it?).  Hence according to the shiny new rules
this work is 2.6.15 material.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/