For example, drivers/base/core.c contains
#define to_root_device(dev) container_of(dev, struct root_device, dev)
which works fine as long as the 'to_root_device' macro is always
applied to a variable called 'dev', as it is the case in the current
kernel sources. However, it breaks as soon as it gets applied to a
variable of any other name, as the name of the variable is also
substituted into the third argument of the 'container_of' macro, which
really should stay 'dev' in the above case.
This patch renames the real macro arguments in all 5 such constructs
found by git-grep -E '#define.*container_of *\( *([^ ,]+) *,.*, *\1 *\)',
which may have missed some similar dangerous constructs, of course.
So these changes probably cross all possible boundaries of responsibility,
and I do not know how to best handle it, suggestions welcome.
Btw. this dangerous idom is also popularised by the excellent Linux Device
Drivers book (3rd edition, chapter 14, bottom of page 383).
Signed-off-by: Ferenc Wagner <[email protected]>
---
arch/x86/kernel/hpet.c | 2 +-
drivers/base/core.c | 2 +-
drivers/mtd/nand/fsl_upm.c | 2 +-
include/linux/i2o.h | 2 +-
include/linux/mtd/sh_flctl.h | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index dedc2bd..48f70ad 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -27,7 +27,7 @@
#define HPET_DEV_FSB_CAP 0x1000
#define HPET_DEV_PERI_CAP 0x2000
-#define EVT_TO_HPET_DEV(evt) container_of(evt, struct hpet_dev, evt)
+#define EVT_TO_HPET_DEV(evtdev) container_of(evtdev, struct hpet_dev, evt)
/*
* HPET address is set in acpi/boot.c, when an ACPI entry exists
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7ecb193..68374f3 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1285,7 +1285,7 @@ struct root_device
struct module *owner;
};
-#define to_root_device(dev) container_of(dev, struct root_device, dev)
+#define to_root_device(d) container_of(d, struct root_device, dev)
static void root_device_release(struct device *dev)
{
diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index d120cd8..623d0b4 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -48,7 +48,7 @@ struct fsl_upm_nand {
uint32_t wait_flags;
};
-#define to_fsl_upm_nand(mtd) container_of(mtd, struct fsl_upm_nand, mtd)
+#define to_fsl_upm_nand(mtdinfo) container_of(mtdinfo, struct fsl_upm_nand, mtd)
static int fun_chip_ready(struct mtd_info *mtd)
{
diff --git a/include/linux/i2o.h b/include/linux/i2o.h
index 4c4e57d..b588b8f 100644
--- a/include/linux/i2o.h
+++ b/include/linux/i2o.h
@@ -782,7 +782,7 @@ extern int i2o_exec_lct_get(struct i2o_controller *);
#define to_i2o_driver(drv) container_of(drv,struct i2o_driver, driver)
#define to_i2o_device(dev) container_of(dev, struct i2o_device, device)
#define to_i2o_controller(dev) container_of(dev, struct i2o_controller, device)
-#define kobj_to_i2o_device(kobj) to_i2o_device(container_of(kobj, struct device, kobj))
+#define kobj_to_i2o_device(kobject) to_i2o_device(container_of(kobject, struct device, kobj))
/**
* i2o_out_to_virt - Turn an I2O message to a virtual address
diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
index e77c1ce..254f5c3 100644
--- a/include/linux/mtd/sh_flctl.h
+++ b/include/linux/mtd/sh_flctl.h
@@ -91,7 +91,7 @@
#define INIT_FL4ECCRESULT_VAL 0x03FF03FF
#define LOOP_TIMEOUT_MAX 0x00010000
-#define mtd_to_flctl(mtd) container_of(mtd, struct sh_flctl, mtd)
+#define mtd_to_flctl(mtdinfo) container_of(mtdinfo, struct sh_flctl, mtd)
struct sh_flctl {
struct mtd_info mtd;
On Mon, 14 Sep 2009 10:27:00 +0200 Ferenc Wagner <[email protected]> wrote:
> For example, drivers/base/core.c contains
>
> #define to_root_device(dev) container_of(dev, struct root_device, dev)
>
> which works fine as long as the 'to_root_device' macro is always
> applied to a variable called 'dev', as it is the case in the current
> kernel sources. However, it breaks as soon as it gets applied to a
> variable of any other name, as the name of the variable is also
> substituted into the third argument of the 'container_of' macro, which
> really should stay 'dev' in the above case.
>
> This patch renames the real macro arguments in all 5 such constructs
> found by git-grep -E '#define.*container_of *\( *([^ ,]+) *,.*, *\1 *\)',
> which may have missed some similar dangerous constructs, of course.
> So these changes probably cross all possible boundaries of responsibility,
> and I do not know how to best handle it, suggestions welcome.
>
> Btw. this dangerous idom is also popularised by the excellent Linux Device
> Drivers book (3rd edition, chapter 14, bottom of page 383).
>
> ...
>
> -#define EVT_TO_HPET_DEV(evt) container_of(evt, struct hpet_dev, evt)
> +#define EVT_TO_HPET_DEV(evtdev) container_of(evtdev, struct hpet_dev, evt)
There is no reason whatsoever for implementing these things as macros
and for the life of me I don't understand why people do this.
If we're going to fix these things then how about we turn them into
C functions thereby making them even safer?
y:/usr/src/linux-2.6.31> grep -r 'define.*container_of' . | wc -l
347
Sigh.
Andrew Morton <[email protected]> writes:
> On Mon, 14 Sep 2009 10:27:00 +0200 Ferenc Wagner <[email protected]> wrote:
>
>> For example, drivers/base/core.c contains
>>
>> #define to_root_device(dev) container_of(dev, struct root_device, dev)
>>
>> which works fine as long as the 'to_root_device' macro is always
>> applied to a variable called 'dev', as it is the case in the current
>> kernel sources. However, it breaks as soon as it gets applied to a
>> variable of any other name, as the name of the variable is also
>> substituted into the third argument of the 'container_of' macro, which
>> really should stay 'dev' in the above case.
>>
>> This patch renames the real macro arguments in all 5 such constructs
>> found by git-grep -E '#define.*container_of *\( *([^ ,]+) *,.*, *\1 *\)',
>> which may have missed some similar dangerous constructs, of course.
>> So these changes probably cross all possible boundaries of responsibility,
>> and I do not know how to best handle it, suggestions welcome.
>>
>> Btw. this dangerous idom is also popularised by the excellent Linux Device
>> Drivers book (3rd edition, chapter 14, bottom of page 383).
>>
>> ...
>>
>> -#define EVT_TO_HPET_DEV(evt) container_of(evt, struct hpet_dev, evt)
>> +#define EVT_TO_HPET_DEV(evtdev) container_of(evtdev, struct hpet_dev, evt)
>
> There is no reason whatsoever for implementing these things as macros
> and for the life of me I don't understand why people do this.
>
> If we're going to fix these things then how about we turn them into
> C functions thereby making them even safer?
>
> y:/usr/src/linux-2.6.31> grep -r 'define.*container_of' . | wc -l
> 347
Quite a bunch, but if deemed useful and acceptable, I'm willing to
convert them into static (inline?) functions as a means of thanking
the kernel developer community. But that surely wouldn't fly as a
single patch.
--
Feri.