Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Tue, 2009-08-11 at 12:57 +0530, vibi sreenivasan wrote:
> Fix following compiler warnings
> 
> net/netlink/genetlink.c: In function `genl_register_mc_group':
> net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function

Well, only if there's no netns which can't happen.

However, technical problems prohibit your patch from being applied.

a) you sent it to the wrong list, copying netdev

> Signed-off-by: vibi sreenivasan, CMS COMPUTERS LTD, INDIA <vibi_sreenivasan@cms.com>

b) this isn't correctly formatted, have you actually read
Documentation/SubmittingPatches including the legal text that you're
agreeing to (section 12 of that document)?

johannes

>  net/netlink/genetlink.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 575c643..66f6ba0 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
>  {
>  	int id;
>  	unsigned long *new_groups;
> -	int err;
> +	int err = 0;
>  
>  	BUG_ON(grp->name[0] == '\0');
>  


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

Hi,


> > > net/netlink/genetlink.c: In function `genl_register_mc_group':
> > > net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function
> > 
> > Well, only if there's no netns which can't happen.

> 	So how to silence that warning?

Well, I suppose your patch is the best way anyhow, using
uninitialized_var() would seem somewhat dangerous for future
modifications of the function.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

[vibi sreenivasan]

Hi Johannes,
On Tue, 2009-08-11 at 09:36 +0200, Johannes Berg wrote:
> On Tue, 2009-08-11 at 12:57 +0530, vibi sreenivasan wrote:
> > Fix following compiler warnings
> > 
> > net/netlink/genetlink.c: In function `genl_register_mc_group':
> > net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function
> 
> Well, only if there's no netns which can't happen.
	So how to silence that warning?
> 
> However, technical problems prohibit your patch from being applied.
> 
> a) you sent it to the wrong list, copying netdev
> 
Sorry i will send it to netdev

> > Signed-off-by: vibi sreenivasan, CMS COMPUTERS LTD, INDIA <vibi_sreenivasan@cms.com>
> 
> b) this isn't correctly formatted, have you actually read
> Documentation/SubmittingPatches including the legal text that you're
> agreeing to (section 12 of that document)?
> 
Sorry again i will remove the tags at the end & resubmit it again.

Thanks for pointing out the issues.

Thanks & Regards
vibi sreenivasan

> johannes
> 
> >  net/netlink/genetlink.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> > index 575c643..66f6ba0 100644
> > --- a/net/netlink/genetlink.c
> > +++ b/net/netlink/genetlink.c
> > @@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
> >  {
> >  	int id;
> >  	unsigned long *new_groups;
> > -	int err;
> > +	int err = 0;
> >  
> >  	BUG_ON(grp->name[0] == '\0');
> >  
> 

Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

[Marcel Holtmann]

Hi Stephen,

> > > > @@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
> > > >  {
> > > >         int id;
> > > >         unsigned long *new_groups;
> > > > -       int err;
> > > > +       int err = 0;
> > > >  
> > > >         BUG_ON(grp->name[0] == '\0');
> > > 
> > > this looks fishy. How does gcc thinks this variable is uninitialized. If
> > > I look at the code in Linus' tree, I don't see it.
> > 
> > Agreed, and the line numbers are off.
> 
> In the -next tree, it looks like this:

it would have been nice if the patch actually indicates that it is for
-next since otherwise just shutting up a compiler warning is a bad idea
in this case.

> int genl_register_mc_group(struct genl_family *family,
> 			   struct genl_multicast_group *grp)
> {
> 	int id;
> 	unsigned long *new_groups;
> 	int err;
> 
> 	BUG_ON(grp->name[0] == '\0');
> 
> 	genl_lock();
> 
> 	/* special-case our own group */
> 	if (grp == &notify_grp)
> 		id = GENL_ID_CTRL;
> 	else
> 		id = find_first_zero_bit(mc_groups,
> 					 mc_groups_longs * BITS_PER_LONG);
> 
> 
> 	if (id >= mc_groups_longs * BITS_PER_LONG) {
> 		size_t nlen = (mc_groups_longs + 1) * sizeof(unsigned long);
> 
> 		if (mc_groups == &mc_group_start) {
> 			new_groups = kzalloc(nlen, GFP_KERNEL);
> 			if (!new_groups) {
> 				err = -ENOMEM;
> 				goto out;
> 			}
> 			mc_groups = new_groups;
> 			*mc_groups = mc_group_start;
> 		} else {
> 			new_groups = krealloc(mc_groups, nlen, GFP_KERNEL);
> 			if (!new_groups) {
> 				err = -ENOMEM;
> 				goto out;
> 			}
> 			mc_groups = new_groups;
> 			mc_groups[mc_groups_longs] = 0;
> 		}
> 		mc_groups_longs++;
> 	}
> 
> 	if (family->netnsok) {
> 		struct net *net;
> 
> 		rcu_read_lock();
> 		for_each_net_rcu(net) {
> 			err = netlink_change_ngroups(net->genl_sock,
> 					mc_groups_longs * BITS_PER_LONG);
> 			if (err) {
> 				rcu_read_unlock();
> 				goto out;
> 			}
> 		}
> 		rcu_read_unlock();
> 	} else {
> 		err = netlink_change_ngroups(init_net.genl_sock,
> 					     mc_groups_longs * BITS_PER_LONG);
> 		if (err)
> 			goto out;
> 	}
> 
> 	grp->id = id;
> 	set_bit(id, mc_groups);
> 	list_add_tail(&grp->list, &family->mcast_groups);
> 	grp->family = family;
> 
> 	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, grp);
>  out:
> 	genl_unlock();
> 	return err;
> }
> 
> so if family->netnsok is true and the for_each_net_rcu loop executes 0
> times, err will not be set ...  Now, that may not be logically possible,
> but I can't tell that from this code.

I prefer we add a err = 0 in the if (family->netnsok) { block instead of
just globally setting it to a value.

Regards

Marcel

Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2432 bytes --]

Hi all,

On Tue, 11 Aug 2009 20:24:59 -0700 Stephen Hemminger <shemminger@vyatta.com> wrote:
>
> > > @@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
> > >  {
> > >         int id;
> > >         unsigned long *new_groups;
> > > -       int err;
> > > +       int err = 0;
> > >  
> > >         BUG_ON(grp->name[0] == '\0');
> > 
> > this looks fishy. How does gcc thinks this variable is uninitialized. If
> > I look at the code in Linus' tree, I don't see it.
> 
> Agreed, and the line numbers are off.

In the -next tree, it looks like this:

int genl_register_mc_group(struct genl_family *family,
			   struct genl_multicast_group *grp)
{
	int id;
	unsigned long *new_groups;
	int err;

	BUG_ON(grp->name[0] == '\0');

	genl_lock();

	/* special-case our own group */
	if (grp == &notify_grp)
		id = GENL_ID_CTRL;
	else
		id = find_first_zero_bit(mc_groups,
					 mc_groups_longs * BITS_PER_LONG);


	if (id >= mc_groups_longs * BITS_PER_LONG) {
		size_t nlen = (mc_groups_longs + 1) * sizeof(unsigned long);

		if (mc_groups == &mc_group_start) {
			new_groups = kzalloc(nlen, GFP_KERNEL);
			if (!new_groups) {
				err = -ENOMEM;
				goto out;
			}
			mc_groups = new_groups;
			*mc_groups = mc_group_start;
		} else {
			new_groups = krealloc(mc_groups, nlen, GFP_KERNEL);
			if (!new_groups) {
				err = -ENOMEM;
				goto out;
			}
			mc_groups = new_groups;
			mc_groups[mc_groups_longs] = 0;
		}
		mc_groups_longs++;
	}

	if (family->netnsok) {
		struct net *net;

		rcu_read_lock();
		for_each_net_rcu(net) {
			err = netlink_change_ngroups(net->genl_sock,
					mc_groups_longs * BITS_PER_LONG);
			if (err) {
				rcu_read_unlock();
				goto out;
			}
		}
		rcu_read_unlock();
	} else {
		err = netlink_change_ngroups(init_net.genl_sock,
					     mc_groups_longs * BITS_PER_LONG);
		if (err)
			goto out;
	}

	grp->id = id;
	set_bit(id, mc_groups);
	list_add_tail(&grp->list, &family->mcast_groups);
	grp->family = family;

	genl_ctrl_event(CTRL_CMD_NEWMCAST_GRP, grp);
 out:
	genl_unlock();
	return err;
}

so if family->netnsok is true and the for_each_net_rcu loop executes 0
times, err will not be set ...  Now, that may not be logically possible,
but I can't tell that from this code.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

[Stephen Hemminger]

On Tue, 11 Aug 2009 16:57:41 -0700
Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Vibi,
> 
> > net/netlink/genetlink.c: In function `genl_register_mc_group':
> > net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function
> > 
> > Signed-off-by: vibi sreenivasan <vibi_sreenivasan@cms.com>
> > ---
> >  net/netlink/genetlink.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> > index 575c643..66f6ba0 100644
> > --- a/net/netlink/genetlink.c
> > +++ b/net/netlink/genetlink.c
> > @@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
> >  {
> >         int id;
> >         unsigned long *new_groups;
> > -       int err;
> > +       int err = 0;
> >  
> >         BUG_ON(grp->name[0] == '\0');
> 
> this looks fishy. How does gcc thinks this variable is uninitialized. If
> I look at the code in Linus' tree, I don't see it.

Agreed, and the line numbers are off.

-- 

Re: [PATCH] Fix Warnings from net/netlink/genetlink.c

[Marcel Holtmann]

Hi Vibi,

> net/netlink/genetlink.c: In function `genl_register_mc_group':
> net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function
> 
> Signed-off-by: vibi sreenivasan <vibi_sreenivasan@cms.com>
> ---
>  net/netlink/genetlink.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 575c643..66f6ba0 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
>  {
>         int id;
>         unsigned long *new_groups;
> -       int err;
> +       int err = 0;
>  
>         BUG_ON(grp->name[0] == '\0');

this looks fishy. How does gcc thinks this variable is uninitialized. If
I look at the code in Linus' tree, I don't see it.

Regards

Marcel

[PATCH] Fix Warnings from net/netlink/genetlink.c

[vibi sreenivasan]

net/netlink/genetlink.c: In function `genl_register_mc_group':
net/netlink/genetlink.c:139: warning: 'err' might be used uninitialized in this function

Signed-off-by: vibi sreenivasan <vibi_sreenivasan@cms.com>
---
 net/netlink/genetlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 575c643..66f6ba0 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
 {
        int id;
        unsigned long *new_groups;
-       int err;
+       int err = 0;
 
        BUG_ON(grp->name[0] == '\0');
 
-- 
1.6.3.3