[PATCH] block/genhd.c: fix sparse warning

[PATCH] block/genhd.c: fix sparse warning

[H Hartley Sweeten]

Fix sparse warning in block/genhd.c.

	warning: symbol '__mptr' shadows an earlier one

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

---

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;
 }
  

Re: [PATCH] block/genhd.c: fix sparse warning

[Vegard Nossum]

2009/4/16 H Hartley Sweeten <hartleys@visionengravers.com>:
> Fix sparse warning in block/genhd.c.
>
>        warning: symbol '__mptr' shadows an earlier one
>
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

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;
>  }

Re: [PATCH] block/genhd.c: fix sparse warning

[Al Viro]

On Thu, Apr 16, 2009 at 06:46:32AM +0200, Vegard Nossum wrote:
> 2009/4/16 H Hartley Sweeten <hartleys@visionengravers.com>:
> > Fix sparse warning in block/genhd.c.
> >
> > ?? ?? ?? ??warning: symbol '__mptr' shadows an earlier one
> >
> > Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> 
> 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.

Re: [PATCH] block/genhd.c: fix sparse warning

[Vegard Nossum]

2009/4/16 Al Viro <viro@zeniv.linux.org.uk>:
> On Thu, Apr 16, 2009 at 06:46:32AM +0200, Vegard Nossum wrote:
>> 2009/4/16 H Hartley Sweeten <hartleys@visionengravers.com>:
>> > Fix sparse warning in block/genhd.c.
>> >
>> > ?? ?? ?? ??warning: symbol '__mptr' shadows an earlier one
>> >
>> > Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>>
>> 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

RE: [PATCH] block/genhd.c: fix sparse warning

[H Hartley Sweeten]

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 <hsweeten@visionengravers.com>
>
> 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

Re: [PATCH] block/genhd.c: fix sparse warning

[Al Viro]

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.