Re: [PATCH] KEYS: Make request_key() and co fundamentally asynchronous
On Mon, 17 Sep 2007 11:56:00 +0100 David Howells <[EMAIL PROTECTED]> wrote:
> > checkpatch generates a pile of warnings, all of which afacit are legit.
>
> For this warning:
>
> ERROR: need space after that ',' (ctx:WxV)
> #627: FILE: security/keys/internal.h:28:
> +#define kenter(FMT, ...) no_printk("==> %s("FMT")\n",__FUNCTION__
> ,##__VA_ARGS__)
>
>^
>
> This is with good reason. Some versions of cpp get the ## resolution "wrong"
> if __VA_ARGS__ is empty (ie: there are no arguments to the macro that
> correspond to the "..."). This can be worked around by abutting the "," the
> "##" and the "__VA_ARGS__" with no spaces, and inserting a space before the
> comma.
Tell me about it - I fixed that about 100 times. Then we upped the
minimum required gcc version so it is no longer a problem.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KEYS: Make request_key() and co fundamentally asynchronous
Andrew Morton <[EMAIL PROTECTED]> wrote:
> So who's going to review this? Nobody? Well gee, maybe it was my turn
> anyway.
Well, Kevin Coffman has reviewed it and tested it against his NFS keys patches.
> checkpatch generates a pile of warnings, all of which afacit are legit.
For this warning:
ERROR: need space after that ',' (ctx:WxV)
#627: FILE: security/keys/internal.h:28:
+#define kenter(FMT, ...) no_printk("==> %s("FMT")\n",__FUNCTION__
,##__VA_ARGS__)
^
This is with good reason. Some versions of cpp get the ## resolution "wrong"
if __VA_ARGS__ is empty (ie: there are no arguments to the macro that
correspond to the "..."). This can be worked around by abutting the "," the
"##" and the "__VA_ARGS__" with no spaces, and inserting a space before the
comma.
If I remember correctly, the comma won't be removed if it doesn't abut the
##__VA_ARGS__, and the comma and everthing that abuts it on its LHS can be
removed if there's also no space. Without the "##", the comma isn't removed.
Consider the result of doing x("a"); where x(y, ...) is #defined to each of the
folowing:
DEFINITION RESULT
=== ===
printk(y, __VA_ARGS__) print(y, );
printk(y ,__VA_ARGS__) print(y ,);
printk(y,##__VA_ARGS__) print();
printk(y, ##__VA_ARGS__)print(y, );
printk(y ,##__VA_ARGS__)print(y );
> It'd be nice to add a comment explaining to the long-suffering reader why
> down_write_nested() is used here.
I'll change the comment to:
/* make sure no one's trying to change or use the key when we mark it
* - we tell lockdep that we might nest because we might be revoking an
* authorisation key whilst holding the sem on a key we've just
* instantiated
*/
> You could actually use kstrdup() here, I think.
I could, but that potentially wastes time in two ways: firstly, there can be an
error before we've finished setting up, and that can lead to us having wasted
the time spent making the copy; and secondly, the call to kstrdup itself wastes
a bit of time because of the extra call made.
On the other hand, the code might end up a bit shorter, and isn't particularly
fast path anyway.
> rka->callout_info gets leaked later on (goto auth_key_revoked)
Fixed.
> Apart from that the changes look reasonable to me, but I am not a suitable
> reviewer for keys, NFS or rxrpc stuff. Who is??
Kevin Coffman and Trond for the NFS stuff, both of whom are CC'd.
David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KEYS: Make request_key() and co fundamentally asynchronous
Andrew Morton [EMAIL PROTECTED] wrote: So who's going to review this? Nobody? Well gee, maybe it was my turn anyway. Well, Kevin Coffman has reviewed it and tested it against his NFS keys patches. checkpatch generates a pile of warnings, all of which afacit are legit. For this warning: ERROR: need space after that ',' (ctx:WxV) #627: FILE: security/keys/internal.h:28: +#define kenter(FMT, ...) no_printk(== %s(FMT)\n,__FUNCTION__ ,##__VA_ARGS__) ^ This is with good reason. Some versions of cpp get the ## resolution wrong if __VA_ARGS__ is empty (ie: there are no arguments to the macro that correspond to the ...). This can be worked around by abutting the , the ## and the __VA_ARGS__ with no spaces, and inserting a space before the comma. If I remember correctly, the comma won't be removed if it doesn't abut the ##__VA_ARGS__, and the comma and everthing that abuts it on its LHS can be removed if there's also no space. Without the ##, the comma isn't removed. Consider the result of doing x(a); where x(y, ...) is #defined to each of the folowing: DEFINITION RESULT === === printk(y, __VA_ARGS__) print(y, ); printk(y ,__VA_ARGS__) print(y ,); printk(y,##__VA_ARGS__) print(); printk(y, ##__VA_ARGS__)print(y, ); printk(y ,##__VA_ARGS__)print(y ); It'd be nice to add a comment explaining to the long-suffering reader why down_write_nested() is used here. I'll change the comment to: /* make sure no one's trying to change or use the key when we mark it * - we tell lockdep that we might nest because we might be revoking an * authorisation key whilst holding the sem on a key we've just * instantiated */ You could actually use kstrdup() here, I think. I could, but that potentially wastes time in two ways: firstly, there can be an error before we've finished setting up, and that can lead to us having wasted the time spent making the copy; and secondly, the call to kstrdup itself wastes a bit of time because of the extra call made. On the other hand, the code might end up a bit shorter, and isn't particularly fast path anyway. rka-callout_info gets leaked later on (goto auth_key_revoked) Fixed. Apart from that the changes look reasonable to me, but I am not a suitable reviewer for keys, NFS or rxrpc stuff. Who is?? Kevin Coffman and Trond for the NFS stuff, both of whom are CC'd. David - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KEYS: Make request_key() and co fundamentally asynchronous
On Mon, 17 Sep 2007 11:56:00 +0100 David Howells [EMAIL PROTECTED] wrote: checkpatch generates a pile of warnings, all of which afacit are legit. For this warning: ERROR: need space after that ',' (ctx:WxV) #627: FILE: security/keys/internal.h:28: +#define kenter(FMT, ...) no_printk(== %s(FMT)\n,__FUNCTION__ ,##__VA_ARGS__) ^ This is with good reason. Some versions of cpp get the ## resolution wrong if __VA_ARGS__ is empty (ie: there are no arguments to the macro that correspond to the ...). This can be worked around by abutting the , the ## and the __VA_ARGS__ with no spaces, and inserting a space before the comma. Tell me about it - I fixed that about 100 times. Then we upped the minimum required gcc version so it is no longer a problem. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KEYS: Make request_key() and co fundamentally asynchronous
On Wed, 12 Sep 2007 15:25:08 +0100
David Howells <[EMAIL PROTECTED]> wrote:
> Make request_key() and co fundamentally asynchronous to make it easier for NFS
> to make use of them. There are now accessor functions that do asynchronous
> constructions, a wait function to wait for construction to complete, and a
> completion function for the key type to indicate completion of construction.
>
> Note that the construction queue is now gone. Instead, keys under
> construction
> are linked in to the appropriate keyring in advance, and that anyone
> encountering one must wait for it to be complete before they can use it. This
> is done automatically for userspace.
>
> The following auxiliary changes are also made:
>
> (1) Key type implementation stuff is split from linux/key.h into
> linux/key-type.h.
>
> (2) AF_RXRPC provides a way to allocate null rxrpc-type keys so that AFS does
> not need to call key_instantiate_and_link() directly.
>
> (3) Documentation.
>
> Signed-off-by: David Howells <[EMAIL PROTECTED]>
> ---
>
> Documentation/keys-request-key.txt | 25 +-
> Documentation/keys.txt | 91 +-
> Documentation/networking/rxrpc.txt |7
> fs/afs/cell.c | 17 -
> include/keys/rxrpc-type.h |2
> include/linux/key-type.h | 112 +++
> include/linux/key.h| 99 +-
> net/rxrpc/af_rxrpc.c |1
> net/rxrpc/ar-key.c | 31 ++
> security/keys/internal.h | 29 +-
> security/keys/key.c| 27 +-
> security/keys/process_keys.c | 16 +
> security/keys/request_key.c| 556
> ++--
> security/keys/request_key_auth.c |9 +
> 14 files changed, 595 insertions(+), 427 deletions(-)
So who's going to review this? Nobody? Well gee, maybe it was my turn
anyway.
checkpatch generates a pile of warnings, all of which afacit are legit.
> +#ifdef __KDEBUG
> +#define kenter(FMT, ...) printk("==> %s("FMT")\n",__FUNCTION__
> ,##__VA_ARGS__)
> +#define kleave(FMT, ...) printk("<== %s()"FMT"\n",__FUNCTION__
> ,##__VA_ARGS__)
> +#define kdebug(FMT, ...) printk(FMT"\n" ,##__VA_ARGS__)
> #else
> -#define kenter(FMT, a...)do {} while(0)
> -#define kleave(FMT, a...)do {} while(0)
> -#define kdebug(FMT, a...)do {} while(0)
> +#define kenter(FMT, ...) no_printk("==> %s("FMT")\n",__FUNCTION__
> ,##__VA_ARGS__)
> +#define kleave(FMT, ...) no_printk("<== %s()"FMT"\n",__FUNCTION__
> ,##__VA_ARGS__)
> +#define kdebug(FMT, ...) no_printk(FMT"\n" ,##__VA_ARGS__)
> #endif
How many different versions of this sort of thing do we have? Sigh.
The debug infrastructure changes don't appear to be related to the intent
of this patch and aren't changelogged. Not that this matters a lot.
> @@ -901,10 +901,9 @@ void key_revoke(struct key *key)
>
> /* make sure no one's trying to change or use the key when we mark
>* it */
> - down_write(>sem);
> - set_bit(KEY_FLAG_REVOKED, >flags);
> -
> - if (key->type->revoke)
> + down_write_nested(>sem, 1);
It'd be nice to add a comment explaining to the long-suffering reader why
down_write_nested() is used here.
> @@ -151,6 +152,12 @@ struct key *request_key_auth_new(struct key *target,
> const char *callout_info)
> kleave(" = -ENOMEM");
> return ERR_PTR(-ENOMEM);
> }
> + rka->callout_info = kmalloc(strlen(callout_info) + 1, GFP_KERNEL);
> + if (!rka->callout_info) {
> + kleave(" = -ENOMEM");
> + kfree(rka);
> + return ERR_PTR(-ENOMEM);
> + }
You could actually use kstrdup() here, I think.
rka->callout_info gets leaked later on (goto auth_key_revoked)
> /* see if the calling process is already servicing the key request of
>* another process */
> @@ -179,7 +186,7 @@ struct key *request_key_auth_new(struct key *target,
> const char *callout_info)
> }
>
> rka->target_key = key_get(target);
> - rka->callout_info = callout_info;
> + strcpy(rka->callout_info, callout_info);
>
> /* allocate the auth key */
> sprintf(desc, "%x", target->serial);
Apart from that the changes look reasonable to me, but I am not a suitable
reviewer for keys, NFS or rxrpc stuff. Who is??
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] KEYS: Make request_key() and co fundamentally asynchronous
On Wed, 12 Sep 2007 15:25:08 +0100
David Howells [EMAIL PROTECTED] wrote:
Make request_key() and co fundamentally asynchronous to make it easier for NFS
to make use of them. There are now accessor functions that do asynchronous
constructions, a wait function to wait for construction to complete, and a
completion function for the key type to indicate completion of construction.
Note that the construction queue is now gone. Instead, keys under
construction
are linked in to the appropriate keyring in advance, and that anyone
encountering one must wait for it to be complete before they can use it. This
is done automatically for userspace.
The following auxiliary changes are also made:
(1) Key type implementation stuff is split from linux/key.h into
linux/key-type.h.
(2) AF_RXRPC provides a way to allocate null rxrpc-type keys so that AFS does
not need to call key_instantiate_and_link() directly.
(3) Documentation.
Signed-off-by: David Howells [EMAIL PROTECTED]
---
Documentation/keys-request-key.txt | 25 +-
Documentation/keys.txt | 91 +-
Documentation/networking/rxrpc.txt |7
fs/afs/cell.c | 17 -
include/keys/rxrpc-type.h |2
include/linux/key-type.h | 112 +++
include/linux/key.h| 99 +-
net/rxrpc/af_rxrpc.c |1
net/rxrpc/ar-key.c | 31 ++
security/keys/internal.h | 29 +-
security/keys/key.c| 27 +-
security/keys/process_keys.c | 16 +
security/keys/request_key.c| 556
++--
security/keys/request_key_auth.c |9 +
14 files changed, 595 insertions(+), 427 deletions(-)
So who's going to review this? Nobody? Well gee, maybe it was my turn
anyway.
checkpatch generates a pile of warnings, all of which afacit are legit.
+#ifdef __KDEBUG
+#define kenter(FMT, ...) printk(== %s(FMT)\n,__FUNCTION__
,##__VA_ARGS__)
+#define kleave(FMT, ...) printk(== %s()FMT\n,__FUNCTION__
,##__VA_ARGS__)
+#define kdebug(FMT, ...) printk(FMT\n ,##__VA_ARGS__)
#else
-#define kenter(FMT, a...)do {} while(0)
-#define kleave(FMT, a...)do {} while(0)
-#define kdebug(FMT, a...)do {} while(0)
+#define kenter(FMT, ...) no_printk(== %s(FMT)\n,__FUNCTION__
,##__VA_ARGS__)
+#define kleave(FMT, ...) no_printk(== %s()FMT\n,__FUNCTION__
,##__VA_ARGS__)
+#define kdebug(FMT, ...) no_printk(FMT\n ,##__VA_ARGS__)
#endif
How many differentprivate versions of this sort of thing do we have? Sigh.
The debug infrastructure changes don't appear to be related to the intent
of this patch and aren't changelogged. Not that this matters a lot.
@@ -901,10 +901,9 @@ void key_revoke(struct key *key)
/* make sure no one's trying to change or use the key when we mark
* it */
- down_write(key-sem);
- set_bit(KEY_FLAG_REVOKED, key-flags);
-
- if (key-type-revoke)
+ down_write_nested(key-sem, 1);
It'd be nice to add a comment explaining to the long-suffering reader why
down_write_nested() is used here.
@@ -151,6 +152,12 @@ struct key *request_key_auth_new(struct key *target,
const char *callout_info)
kleave( = -ENOMEM);
return ERR_PTR(-ENOMEM);
}
+ rka-callout_info = kmalloc(strlen(callout_info) + 1, GFP_KERNEL);
+ if (!rka-callout_info) {
+ kleave( = -ENOMEM);
+ kfree(rka);
+ return ERR_PTR(-ENOMEM);
+ }
You could actually use kstrdup() here, I think.
rka-callout_info gets leaked later on (goto auth_key_revoked)
/* see if the calling process is already servicing the key request of
* another process */
@@ -179,7 +186,7 @@ struct key *request_key_auth_new(struct key *target,
const char *callout_info)
}
rka-target_key = key_get(target);
- rka-callout_info = callout_info;
+ strcpy(rka-callout_info, callout_info);
/* allocate the auth key */
sprintf(desc, %x, target-serial);
Apart from that the changes look reasonable to me, but I am not a suitable
reviewer for keys, NFS or rxrpc stuff. Who is??
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
