On Sat, Feb 23, 2008 at 04:59:44PM +0800, WANG Cong wrote:
> From: Alexey Dobriyan
> Subject: Re: [PATCH 00/16] (Resend) Use get_personality()

> > On Sat, Feb 23, 2008 at 04:14:03PM +0800, WANG Cong wrote:
> > > This patchset makes the macro get_personality function alike
> > > and teaches code to use get_personality() instead of explicit
> > > reference.
> > >
> > > [I am sorry if you've received multiple copied of this, since
> > > my git-send-email doesn't work well. ]
> >
> > Yes, but why? "current->personality" is way more understandable than
> > your macro because task subject to dereference is very visible.
>
> Use get_personality() can hide the task_struct internals a bit.

->personality is going to become something less trivial?
Sorry, but you sound like C++ people writing tons of pointless get/set
wrappers. And your get_personality() is worse -- C++ would write it as

current->personality()

and again, even here, it's immediately visible that current task is
involved, not some other task.

> And I don't think using the macro to access it is less understandable.
> Since 'current' won't be NULL, whether the dereference is visible is
> not important.

It's not about NULL vs non-NULL at all.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

* WANG Cong wrote:

> Use get_personality() macro instead of explicit reference for x86
> code.

thanks, applied.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Alexey Dobriyan
Subject: Re: [PATCH 00/16] (Resend) Use get_personality()
Date: Sat, 23 Feb 2008 12:27:10 +0300
Message-ID: <20080223092710.GD2262@martell.zuzino.mipt.ru>

> On Sat, Feb 23, 2008 at 04:59:44PM +0800, WANG Cong wrote:
> > From: Alexey Dobriyan
> > Subject: Re: [PATCH 00/16] (Resend) Use get_personality()
>
> > > On Sat, Feb 23, 2008 at 04:14:03PM +0800, WANG Cong wrote:
> > > > This patchset makes the macro get_personality function alike
> > > > and teaches code to use get_personality() instead of explicit
> > > > reference.
> > > >
> > > > [I am sorry if you've received multiple copied of this, since
> > > > my git-send-email doesn't work well. ]
> > >
> > > Yes, but why? "current->personality" is way more understandable than
> > > your macro because task subject to dereference is very visible.
> >
> > Use get_personality() can hide the task_struct internals a bit.
>
> ->personality is going to become something less trivial?
> Sorry, but you sound like C++ people writing tons of pointless get/set
> wrappers. And your get_personality() is worse -- C++ would write it as
>
> current->personality()
>
> and again, even here, it's immediately visible that current task is
> involved, not some other task.
>

Can't get_personality() mean getting the personality of current task?

Or you want a more generic macro like this?

#define get_task_personality(tsk) ((tsk)->personality)

No, that is _too_ generic. Look at the code, (nearly) all references to
'personality' are via 'current'. So get_personality() is enough.

I am not a fan of C++, I know that sometimes the get/set method in C++
is really a bit pointless, but, of course, *not* all the times.

Regards.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

* Ingo Molnar wrote:

> * WANG Cong wrote:
>
> > Use get_personality() macro instead of explicit reference for x86
> > code.
>
> thanks, applied.

randconfig testing found the following build failures:

arch/x86/kernel/process_64.c: In function 'do_arch_prctl':
arch/x86/kernel/process_64.c:836: error: called object 'get_current()->personality' is not a function
arch/x86/kernel/process_64.c:862: error: called object 'get_current()->personality' is not a function

so i dropped the patch again. Config attached.

Ingo

From: Ingo Molnar
Subject: Re: [PATCH 08/16] x86: use get_personality()
Date: Sat, 23 Feb 2008 11:19:54 +0100
Message-ID: <20080223101954.GA28929@elte.hu>

>
> * Ingo Molnar wrote:
>
> > * WANG Cong wrote:
> >
> > > Use get_personality() macro instead of explicit reference for x86
> > > code.
> >
> > thanks, applied.
>
> randconfig testing found the following build failures:
>
> arch/x86/kernel/process_64.c: In function 'do_arch_prctl':
> arch/x86/kernel/process_64.c:836: error: called object 'get_current()->personality' is not a function
> arch/x86/kernel/process_64.c:862: error: called object 'get_current()->personality' is not a function
>
> so i dropped the patch again. Config attached.
>
> Ingo

Sorry that I didn't mention that this patch should be applied
_after_ [patch 01/16], since in [patch 01/16] I made the macro
get_personality like a function.

Sorry for this. Just wait after [patch 01/16] is applied.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

WANG Cong wrote:

> Use get_personality() macro instead of explicit reference
> for frv code.
>
> Signed-off-by: WANG Cong
Acked-By: David Howells
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

WANG Cong wrote:

> This patch makes the macro get_personality function-like.
>
> Signed-off-by: WANG Cong
Acked-By: David Howells
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

WANG Cong wrote:

> Use get_personality() macro instead of explicit reference
> for mn10300 code.
>
> Signed-off-by: WANG Cong
Acked-By: David Howells
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Matthew Wilcox wrote:
> On Sat, Feb 23, 2008 at 04:14:08PM +0800, WANG Cong wrote:
>
>> Use get_personality() macro instead of explicit reference
>> for parisc code.
>> - if (personality(current->personality) == PER_LINUX32
>> + if (personality(get_personality()) == PER_LINUX32
>>
>
> Hm. We have an interesting clash of conventions here.
>
> On the one hand, we have the java-style accessor convention.
> get_personality()/set_personality().
>

Is get_personality() thought to be better than current->personality? To
me, the latter seems more meaningful, and I'd rather see it than the former.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sat, Feb 23, 2008 at 11:16:22PM +1030, David Newall wrote:
> Is get_personality() thought to be better than current->personality? To
> me, the latter seems more meaningful, and I'd rather see it than the former.

I suppose it's possible we might want to move 'personality' out of
'current' one day, and having an accessor macro makes it easier to do
that. I agree it doesn't seem like a big win though.

--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sat, Feb 23, 2008 at 12:27:10PM +0300, Alexey Dobriyan wrote:
> > Use get_personality() can hide the task_struct internals a bit.
>
> ->personality is going to become something less trivial?
> Sorry, but you sound like C++ people writing tons of pointless get/set
> wrappers. And your get_personality() is worse -- C++ would write it as
>
> current->personality()
>
> and again, even here, it's immediately visible that current task is
> involved, not some other task.

Yes, completely agreement. While I might have introduced this gem
back then it is entirely stupid if you think about it. Please send
patches to kill get_personality and just use current->personality
instead.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Sat, 23 Feb 2008 13:37:31 -0500 Christoph Hellwig wrote:

> On Sat, Feb 23, 2008 at 12:27:10PM +0300, Alexey Dobriyan wrote:
> > > Use get_personality() can hide the task_struct internals a bit.
> >
> > ->personality is going to become something less trivial?
> > Sorry, but you sound like C++ people writing tons of pointless get/set
> > wrappers. And your get_personality() is worse -- C++ would write it as
> >
> > current->personality()
> >
> > and again, even here, it's immediately visible that current task is
> > involved, not some other task.
>
> Yes, completely agreement. While I might have introduced this gem
> back then it is entirely stupid if you think about it. Please send
> patches to kill get_personality and just use current->personality
> instead.
>

yup.

We'll generally only add wrappers of this form if we need to provide
alternative implementations, or if we expect that we shall do so in the
future.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Andrew Morton
Subject: Re: [PATCH 00/16] (Resend) Use get_personality()
Date: Sat, 23 Feb 2008 11:16:29 -0800
Message-ID: <20080223111629.4d8d2c7b.akpm@linux-foundation.org>

> On Sat, 23 Feb 2008 13:37:31 -0500 Christoph Hellwig wrote:
>
> > On Sat, Feb 23, 2008 at 12:27:10PM +0300, Alexey Dobriyan wrote:
> > > > Use get_personality() can hide the task_struct internals a bit.
> > >
> > > ->personality is going to become something less trivial?
> > > Sorry, but you sound like C++ people writing tons of pointless get/set
> > > wrappers. And your get_personality() is worse -- C++ would write it as
> > >
> > > current->personality()
> > >
> > > and again, even here, it's immediately visible that current task is
> > > involved, not some other task.
> >
> > Yes, completely agreement. While I might have introduced this gem
> > back then it is entirely stupid if you think about it. Please send
> > patches to kill get_personality and just use current->personality
> > instead.
> >
>
> yup.
>
> We'll generally only add wrappers of this form if we need to provide
> alternative implementations, or if we expect that we shall do so in the
> future.
>

Ok. I will send a patch to remove it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: WANG Cong
Date: Sun, 24 Feb 2008 12:09:03 +0800
Subject: [PATCH] Remove the macro get_personality

Remove the macro get_personality, use ->personality instead.

Cc: Ingo Molnar
Cc: Christoph Hellwig Cc: Alexey Dobriyan
Cc: David Howells
Cc: Bryan Wu
Signed-off-by: WANG Cong
---
arch/blackfin/kernel/signal.c | 2 +-
arch/frv/kernel/signal.c | 4 ++--
include/linux/personality.h | 4 ----
3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/blackfin/kernel/signal.c b/arch/blackfin/kernel/signal.c
index 5564c95..e8d3869 100644
--- a/arch/blackfin/kernel/signal.c
+++ b/arch/blackfin/kernel/signal.c
@@ -224,7 +224,7 @@ setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t * info,

/* Set up registers for signal handler */
wrusp((unsigned long)frame);
- if (get_personality & FDPIC_FUNCPTRS) {
+ if (current->personality & FDPIC_FUNCPTRS) {
struct fdpic_func_descriptor __user *funcptr =
(struct fdpic_func_descriptor *) ka->sa.sa_handler;
__get_user(regs->pc, &funcptr->text);
diff --git a/arch/frv/kernel/signal.c b/arch/frv/kernel/signal.c
index d64bcaf..3bdb368 100644
--- a/arch/frv/kernel/signal.c
+++ b/arch/frv/kernel/signal.c
@@ -297,7 +297,7 @@ static int setup_frame(int sig, struct k_sigaction *ka, sigset_t *set)
__frame->lr = (unsigned long) &frame->retcode;
__frame->gr8 = sig;

- if (get_personality & FDPIC_FUNCPTRS) {
+ if (current->personality & FDPIC_FUNCPTRS) {
struct fdpic_func_descriptor __user *funcptr =
(struct fdpic_func_descriptor __user *) ka->sa.sa_handler;
__get_user(__frame->pc, &funcptr->text);
@@ -396,7 +396,7 @@ static int setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
__frame->gr8 = sig;
__frame->gr9 = (unsigned long) &frame->info;

- if (get_personality & FDPIC_FUNCPTRS) {
+ if (current->personality & FDPIC_FUNCPTRS) {
struct fdpic_func_descriptor __user *funcptr =
(struct fdpic_func_descriptor __user *) ka->sa.sa_handler;
__get_user(__frame->pc, &funcptr->text);
diff --git a/include/linux/personality.h b/include/linux/personality.h
index 012cd55..a84e9ff 100644
--- a/include/linux/personality.h
+++ b/include/linux/personality.h
@@ -105,10 +105,6 @@ struct exec_domain {
*/
#define personality(pers) (pers & PER_MASK)

-/*
- * Personality of the currently running process.
- */
-#define get_personality (current->personality)

/*
* Change personality of the currently running process.
--
1.5.3.8

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/