[PATCH 2/2] Replace the dangerous to_root_device macro with an inline function

[PATCH 2/2] Replace the dangerous to_root_device macro with an inline function

[Ferenc Wagner]

The original macro worked only when applied to variables named 'dev'.
While this could have been fixed by simply renaming the macro argument,
a more type-safe replacement is preferred.

Signed-off-by: Ferenc Wagner <wferi@niif.hu>
---
 drivers/base/core.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6ed6454..742a78a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1283,7 +1283,10 @@ struct root_device
 	struct module *owner;
 };
 
-#define to_root_device(dev) container_of(dev, struct root_device, dev)
+inline struct root_device *to_root_device(struct device *d)
+{
+	return container_of(d, struct root_device, dev);
+}
 
 static void root_device_release(struct device *dev)
 {
-- 
1.6.5

Re: [PATCH 2/2] Replace the dangerous to_root_device macro with an inline function

[Ferenc Wagner]

Sorry for the misleading 2/2 subject tag, this is a solitary patch.
-- 
Regards,
Feri.

Re: [PATCH 2/2] Replace the dangerous to_root_device macro with an inline function

[Greg KH]

On Thu, Jan 06, 2011 at 04:24:24PM +0100, Ferenc Wagner wrote:
> The original macro worked only when applied to variables named 'dev'.
> While this could have been fixed by simply renaming the macro argument,
> a more type-safe replacement is preferred.

Preferred for what?  This is a local macro, not exported to anything
else, is it causing problems today within this single file?

thanks,

greg k-h

Re: [PATCH 2/2] Replace the dangerous to_root_device macro with an inline function

[Ferenc Wagner]

Greg KH <gregkh@suse.de> writes:

> On Thu, Jan 06, 2011 at 04:24:24PM +0100, Ferenc Wagner wrote:
>
>> The original macro worked only when applied to variables named 'dev'.
>> While this could have been fixed by simply renaming the macro argument,
>> a more type-safe replacement is preferred.
>
> Preferred for what?  This is a local macro, not exported to anything
> else, is it causing problems today within this single file?

No, it isn't a problem currently, because this macro is applied to
variables named 'dev' exclusively.  But the macro definition is wrong,
as it uses 'dev' (the macro argument) in two places, of which the second
probably isn't intentional, as the third argument of the container_of
macro is a structure member name, which should stay fixed to the literal
string 'dev' in this case, not replaced by the actual macro argument.

So you've got the choice of leaving things as they are and happen to
work for the moment, fixing the definition of the to_root_device macro
to use some other token as the name of the free variable instead of
'dev', or to replace the macro with a function, which provides some
type-checking advantages during compilation as well.  I thought this was
a general reason for preferring inline functions over preprocessor
macros in the kernel codebase, but maybe I was wrong.
-- 
Regards,
Feri.

Re: [PATCH 2/2] Replace the dangerous to_root_device macro with an inline function

[Greg KH]

On Thu, Jan 06, 2011 at 09:47:08PM +0100, Ferenc Wagner wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Thu, Jan 06, 2011 at 04:24:24PM +0100, Ferenc Wagner wrote:
> >
> >> The original macro worked only when applied to variables named 'dev'.
> >> While this could have been fixed by simply renaming the macro argument,
> >> a more type-safe replacement is preferred.
> >
> > Preferred for what?  This is a local macro, not exported to anything
> > else, is it causing problems today within this single file?
> 
> No, it isn't a problem currently, because this macro is applied to
> variables named 'dev' exclusively.  But the macro definition is wrong,
> as it uses 'dev' (the macro argument) in two places, of which the second
> probably isn't intentional, as the third argument of the container_of
> macro is a structure member name, which should stay fixed to the literal
> string 'dev' in this case, not replaced by the actual macro argument.

No, it was intentional :)

> So you've got the choice of leaving things as they are and happen to
> work for the moment, fixing the definition of the to_root_device macro
> to use some other token as the name of the free variable instead of
> 'dev', or to replace the macro with a function, which provides some
> type-checking advantages during compilation as well.  I thought this was
> a general reason for preferring inline functions over preprocessor
> macros in the kernel codebase, but maybe I was wrong.

No, you are correct, but as this is a macro that is limited to a single
file, and there's no problem with it as-is today, the need to change it
is quite low.

I'll take this as a general "clean up" patch, if you resend it with the
2/2 removed.  But note that it will not get added to my trees until
after the .38-rc1 merge happens.

thanks,

greg k-h

[PATCH] Replace the dangerous to_root_device macro with an inline function

[Ferenc Wagner]

The original macro worked only when applied to variables named 'dev'.
While this could have been fixed by simply renaming the macro argument,
a more type-safe replacement by an inline function is preferred.

Signed-off-by: Ferenc Wagner <wferi@niif.hu>
---
 drivers/base/core.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6ed6454..742a78a 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1283,7 +1283,10 @@ struct root_device
 	struct module *owner;
 };
 
-#define to_root_device(dev) container_of(dev, struct root_device, dev)
+inline struct root_device *to_root_device(struct device *d)
+{
+	return container_of(d, struct root_device, dev);
+}
 
 static void root_device_release(struct device *dev)
 {
-- 
1.6.5

Re: [PATCH 2/2] Replace the dangerous to_root_device macro with an inline function

[Ferenc Wagner]

Greg KH <gregkh@suse.de> writes:

> On Thu, Jan 06, 2011 at 09:47:08PM +0100, Ferenc Wagner wrote:
>
>> Greg KH <gregkh@suse.de> writes:
>> 
>>> On Thu, Jan 06, 2011 at 04:24:24PM +0100, Ferenc Wagner wrote:
>>>
>>>> The original macro worked only when applied to variables named 'dev'.
>>>> While this could have been fixed by simply renaming the macro argument,
>>>> a more type-safe replacement is preferred.
>>>
>>> Preferred for what?  This is a local macro, not exported to anything
>>> else, is it causing problems today within this single file?
>> 
>> No, it isn't a problem currently, because this macro is applied to
>> variables named 'dev' exclusively.  But the macro definition is wrong,
>> as it uses 'dev' (the macro argument) in two places, of which the second
>> probably isn't intentional, as the third argument of the container_of
>> macro is a structure member name, which should stay fixed to the literal
>> string 'dev' in this case, not replaced by the actual macro argument.
>
> No, it was intentional :)

Then we should fix it's misleading name. :)

> I'll take this as a general "clean up" patch

Great, that's exactly what it is.

> if you resend it with the 2/2 removed.

On its way...

> But note that it will not get added to my trees until after the
> .38-rc1 merge happens.

No problem, it's been sitting in my tree for almost a year now. :)
-- 
Thanks,
Feri.