Fix sparse warning in block/genhd.c.
warning: symbol '__mptr' shadows an earlier one
Signed-off-by: H Hartley Sweeten <[email protected]>
---
diff --git a/block/genhd.c b/block/genhd.c
index a9ec910..ca3eedb 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -576,8 +576,10 @@ struct gendisk *get_gendisk(dev_t devt, int
*partno)
struct kobject *kobj;
kobj = kobj_lookup(bdev_map, devt, partno);
- if (kobj)
- disk = dev_to_disk(kobj_to_dev(kobj));
+ if (kobj) {
+ struct device *dev = kobj_to_dev(kobj);
+ disk = dev_to_disk(dev);
+ }
} else {
struct hd_struct *part;
@@ -1010,7 +1012,7 @@ static int diskstats_show(struct seq_file *seqf,
void *v)
"wsect wuse running use aveq"
"\n\n");
*/
-
+
disk_part_iter_init(&piter, gp, DISK_PITER_INCL_PART0);
while ((hd = disk_part_iter_next(&piter))) {
cpu = part_stat_lock();
@@ -1034,7 +1036,7 @@ static int diskstats_show(struct seq_file *seqf,
void *v)
);
}
disk_part_iter_exit(&piter);
-
+
return 0;
}
2009/4/16 H Hartley Sweeten <[email protected]>:
> Fix sparse warning in block/genhd.c.
>
> warning: symbol '__mptr' shadows an earlier one
>
> Signed-off-by: H Hartley Sweeten <[email protected]>
Hi,
Just a heads up: There seems to be some sort of consensus that this
type of patch title is not a very good one. (What about "remove
variable shadowing"?)
It would also be nice to have an explanation of where the __mptr
symbol comes into play, because it doesn't even appear in the patch,
and reviewers would likely have an easier job if they knew where to
look it up.
Was this warning harmless, or was the code in fact broken?
Can we rewrite container_of() to not use an extra variable (__mptr),
or perhaps using an inline function for part of the computation?
Do we also have this problem in expressions like max(max(x, y), z)?
Thanks :-)
Vegard
>
> ---
>
> diff --git a/block/genhd.c b/block/genhd.c
> index a9ec910..ca3eedb 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -576,8 +576,10 @@ struct gendisk *get_gendisk(dev_t devt, int
> *partno)
> struct kobject *kobj;
>
> kobj = kobj_lookup(bdev_map, devt, partno);
> - if (kobj)
> - disk = dev_to_disk(kobj_to_dev(kobj));
> + if (kobj) {
> + struct device *dev = kobj_to_dev(kobj);
> + disk = dev_to_disk(dev);
> + }
> } else {
> struct hd_struct *part;
>
> @@ -1010,7 +1012,7 @@ static int diskstats_show(struct seq_file *seqf,
> void *v)
> "wsect wuse running use aveq"
> "\n\n");
> */
> -
> +
> disk_part_iter_init(&piter, gp, DISK_PITER_INCL_PART0);
> while ((hd = disk_part_iter_next(&piter))) {
> cpu = part_stat_lock();
> @@ -1034,7 +1036,7 @@ static int diskstats_show(struct seq_file *seqf,
> void *v)
> );
> }
> disk_part_iter_exit(&piter);
> -
> +
> return 0;
> }
On Thu, Apr 16, 2009 at 06:46:32AM +0200, Vegard Nossum wrote:
> 2009/4/16 H Hartley Sweeten <[email protected]>:
> > Fix sparse warning in block/genhd.c.
> >
> > ?? ?? ?? ??warning: symbol '__mptr' shadows an earlier one
> >
> > Signed-off-by: H Hartley Sweeten <[email protected]>
>
> Hi,
>
> Just a heads up: There seems to be some sort of consensus that this
> type of patch title is not a very good one. (What about "remove
> variable shadowing"?)
Indeed. Speaking of other kind of unpleasantness, could you please
configure your mail-luser-agent to *NOT* replace tabs with series
of spaces/nbsp? That crap is very annoying - these strings of
U+00A0 U+00A0 U+00A0 et sodding cetera are gratuitously making life
painful for non-UTF8 terminals.
2009/4/16 Al Viro <[email protected]>:
> On Thu, Apr 16, 2009 at 06:46:32AM +0200, Vegard Nossum wrote:
>> 2009/4/16 H Hartley Sweeten <[email protected]>:
>> > Fix sparse warning in block/genhd.c.
>> >
>> > ?? ?? ?? ??warning: symbol '__mptr' shadows an earlier one
>> >
>> > Signed-off-by: H Hartley Sweeten <[email protected]>
>>
>> Hi,
>>
>> Just a heads up: There seems to be some sort of consensus that this
>> type of patch title is not a very good one. (What about "remove
>> variable shadowing"?)
>
> Indeed. Speaking of other kind of unpleasantness, could you please
> configure your mail-luser-agent to *NOT* replace tabs with series
> of spaces/nbsp? That crap is very annoying - these strings of
> U+00A0 U+00A0 U+00A0 et sodding cetera are gratuitously making life
> painful for non-UTF8 terminals.
Oh, was that my fault?
That's strange, is this something new in Gmail? Can't remember seeing
this before. It seems that something changed; compare:
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
before to
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
...hm...
I'll try to fix it, thanks.
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
On Wednesday, April 15, 2009 9:47 PM, Vegard Nossum wrote:
>> Fix sparse warning in block/genhd.c.
>>
>> ? ? ? ?warning: symbol '__mptr' shadows an earlier one
>>
>> Signed-off-by: H Hartley Sweeten <[email protected]>
>
> Hi,
>
> Just a heads up: There seems to be some sort of consensus that
> this type of patch title is not a very good one. (What about
> "remove variable shadowing"?)
Sorry, I wasn't aware of that. I just started using sparse and was
cleaning up some warnings in the arch/arm/mach-ep93xx branch. I
noticed this warning and it seemed like a simple fix.
> It would also be nice to have an explanation of where the __mptr
> symbol comes into play, because it doesn't even appear in the
> patch, and reviewers would likely have an easier job if they
> knew where to look it up.
The only __mptr symbol in the source is in the container_of() macro
in include/linux/kernel.h:
#define container_of(ptr, type, member) ({ \
const typeof( ((type *)0)->member ) *__mptr = (ptr); \
(type *)( (char *)__mptr - offsetof(type,member) );})
The sparse warning shows up when a macro expansion ends up with
something like:
type1 val = container_of(container_of(ptr2, type2, member2),
type1, member1);
> Was this warning harmless, or was the code in fact broken?
Should be harmless, the scope of __mptr should only be in the macro.
> Can we rewrite container_of() to not use an extra variable (__mptr),
> or perhaps using an inline function for part of the computation?
Maybe something like:
#define container_of(ptr, type, member) ({ \
const typeof( ((type *)0)->member ) *__m_##ptr = (ptr); \
(type *)( (char *)__m_##ptr - offsetof(type,member) );})
I don't know if that actually works. ;-)
> Do we also have this problem in expressions like max(max(x, y), z)?
That should generate the same sparse warning since each max() has a
couple of local variables (_min1 and _min2).
> Thanks :-)
Not a problem. Just trying to help!
Regards,
Hartley
On Thu, Apr 16, 2009 at 02:03:39PM -0400, H Hartley Sweeten wrote:
> Maybe something like:
>
> #define container_of(ptr, type, member) ({ \
> const typeof( ((type *)0)->member ) *__m_##ptr = (ptr); \
> (type *)( (char *)__m_##ptr - offsetof(type,member) );})
>
> I don't know if that actually works. ;-)
_Think_ of it. Will do bleeding wonders if you say
container_of(f(), struct foo, bar)
won't it?
No, the real solution for that is this:
#define container_of(ptr, type, member) (type *) \
((char *)(1 ? (ptr) : ((const typeof(((type *)0)->member) *)(ptr))) \
- offsetof(type, member))
Same amount of typechecking, no local variables, no ({ }) uses.