Messages in this thread | | | From | David Howells <> | Subject | Re: [PATCH v2 2/2] cred: change keyctl_session_to_parent() to use task_work_queue() | Date | Thu, 12 Apr 2012 10:29:55 +0100 |
| |
Oleg Nesterov <oleg@redhat.com> wrote:
> Change keyctl_session_to_parent() to use task_work_queue() and > move key_replace_session_keyring() logic into task_work->func().
I'm generally okay with this, but there are a couple of issues with the patch.
> +static void replace_session_keyring(struct task_work *twork)
Can you keep this in process_keys.c please? Then everything that actually updates a process's keyrings is done there. Admittedly, on that basis, you can argue that I should move a chunk of keyctl_session_to_parent() there too.
And, also, can you please keep the "key_" on the front of the name?
> long keyctl_session_to_parent(void) > { > -#ifdef TIF_NOTIFY_RESUME
Unless TIF_NOTIFY_RESUME is defined, this operation cannot be performed and should generate an error. I don't see how this happens now.
> + if (!task_work_queue(parent, newwork))
I hate this type of construct. "if not function()" indicating the function succeeded. Can you make it "== 0" instead? Also, shouldn't we tell the user that it failed?
David
| |