| Message ID | 1249985224.13065.11.camel@HunTEr |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 == ¬ify_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.
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 == ¬ify_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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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');
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(-)