Skip to contentSkip to conversation

Fix Warnings from net/netlink/genetlink.c

PATCH In review vibi sreenivasan 4 participants 5 messages Aug 11, 2009 – Aug 12, 2009

Subsystems (3)
Networking Network drivers Linux-next tree
lore
Share & tools
lore

vibi sreenivasan

author
#0 lore raw

PATCH 1/1 Fix Warnings from net/netlink/genetlink.c

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 <[email protected]>
---
 net/netlink/genetlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/netlink/genetlink.c +1 -1
index 575c643..66f6ba0 100644
@@ -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');
Signature · 1 line
1.6.3.3
#1 lore raw

Hi Vibi,

Quoted 13 lines

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 <[email protected]>
---
 net/netlink/genetlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/netlink/genetlink.c +0 -0
index 575c643..66f6ba0 100644

net/netlink/genetlink.c int genl_register_mc_group(struct genl_family *family, · line 141

Quoted 7 lines
net/netlink/genetlink.c +1 -1
@@ -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

#2 lore raw
Quoted 21 lines · On Tue, 11 Aug 2009 16:57:41 -0700 Marcel Holtmann <[email protected]> wrote:

Hi Vibi,

Quoted 19 lines

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 <[email protected]>
---
 net/netlink/genetlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/netlink/genetlink.c +1 -1
index 575c643..66f6ba0 100644
@@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
 {
        int id;
        unsigned long *new_groups;
-       int err;
+       int err = 0;
Quoted 4 lines

net/netlink/genetlink.c int genl_register_mc_group(struct genl_family *family, · line 141

Quoted 1 line
net/netlink/genetlink.c +0 -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.

#3 lore raw

Hi all,

Quoted 8 lines · On Tue, 11 Aug 2009 20:24:59 -0700 Stephen Hemminger <[email protected]>…
Quoted 8 lines
Quoted 8 lines
net/netlink/genetlink.c +1 -1
@@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
 {
        int id;
        unsigned long *new_groups;
-       int err;
+       int err = 0;
Quoted 6 lines
Quoted 4 lines

net/netlink/genetlink.c int genl_register_mc_group(struct genl_family *family, · line 141

Quoted 1 line
net/netlink/genetlink.c +0 -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.

Signature · 3 lines
Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/
#4 lore raw

Hi Stephen,

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
net/netlink/genetlink.c +1 -1
@@ -136,7 +136,7 @@ int genl_register_mc_group(struct genl_family *family,
 {
        int id;
        unsigned long *new_groups;
-       int err;
+       int err = 0;
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

net/netlink/genetlink.c int genl_register_mc_group(struct genl_family *family, · line 141

Quoted 1 line
net/netlink/genetlink.c +0 -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.

Quoted 73 lines

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;
}
Quoted 3 lines

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