Commit 053c095a authored Jan 16, 2015 by Johannes Berg Committed by David S. Miller Jan 18, 2015

netlink: make nlmsg_end() and genlmsg_end() void



Contrary to common expectations for an "int" return, these functions
return only a positive value -- if used correctly they cannot even
return 0 because the message header will necessarily be in the skb.

This makes the very common pattern of

  if (genlmsg_end(...) < 0) { ... }

be a whole bunch of dead code. Many places also simply do

  return nlmsg_end(...);

and the caller is expected to deal with it.

This also commonly (at least for me) causes errors, because it is very
common to write

  if (my_function(...))
    /* error condition */

and if my_function() does "return nlmsg_end()" this is of course wrong.

Additionally, there's not a single place in the kernel that actually
needs the message length returned, and if anyone needs it later then
it'll be very easy to just use skb->len there.

Remove this, and make the functions void. This removes a bunch of dead
code as described above. The patch adds lines because I did

-	return nlmsg_end(...);
+	nlmsg_end(...);
+	return 0;

I could have preserved all the function's return values by returning
skb->len, but instead I've audited all the places calling the affected
functions and found that none cared. A few places actually compared
the return value with <= 0 in dump functionality, but that could just
be changed to < 0 with no change in behaviour, so I opted for the more
efficient version.

One instance of the error I've made numerous times now is also present
in net/phonet/pn_netlink.c in the route_dumpit() function - it didn't
check for <0 or <=0 and thus broke out of the loop every single time.
I've preserved this since it will (I think) have caused the messages to
userspace to be formatted differently with just a single message for
every SKB returned to userspace. It's possible that this isn't needed
for the tools that actually use this, but I don't even know what they
are so couldn't test that changing this behaviour would be acceptable.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

parent ede58ef2

drivers/acpi/event.c

+1 −6

Original line number	Diff line number	Diff line
		@@ -100,7 +100,6 @@ int acpi_bus_generate_netlink_event(const char *device_class,
		struct acpi_genl_event *event;
		void *msg_header;
		int size;
		int result;

		/* allocate memory */
		size = nla_total_size(sizeof(struct acpi_genl_event)) +
		@@ -137,11 +136,7 @@ int acpi_bus_generate_netlink_event(const char *device_class,
		event->data = data;

		/* send multicast genetlink message */
		result = genlmsg_end(skb, msg_header);
		if (result < 0) {
		nlmsg_free(skb);
		return result;
		}
		genlmsg_end(skb, msg_header);

		genlmsg_multicast(&acpi_event_genl_family, skb, 0, 0, GFP_ATOMIC);
		return 0;

drivers/net/ethernet/rocker/rocker.c

+2 −1

Original line number	Diff line number	Diff line
		@@ -3674,7 +3674,8 @@ static int rocker_fdb_fill_info(struct sk_buff *skb,
		if (vid && nla_put_u16(skb, NDA_VLAN, vid))
		goto nla_put_failure;

		return nlmsg_end(skb, nlh);
		nlmsg_end(skb, nlh);
		return 0;

		nla_put_failure:
		nlmsg_cancel(skb, nlh);

drivers/net/vxlan.c

+2 −1

Original line number	Diff line number	Diff line
		@@ -363,7 +363,8 @@ static int vxlan_fdb_info(struct sk_buff skb, struct vxlan_dev vxlan,
		if (nla_put(skb, NDA_CACHEINFO, sizeof(ci), &ci))
		goto nla_put_failure;

		return nlmsg_end(skb, nlh);
		nlmsg_end(skb, nlh);
		return 0;

		nla_put_failure:
		nlmsg_cancel(skb, nlh);

drivers/net/wireless/mac80211_hwsim.c

+2 −1

Original line number	Diff line number	Diff line
		@@ -2557,7 +2557,8 @@ static int mac80211_hwsim_get_radio(struct sk_buff *skb,
		if (res < 0)
		goto out_err;

		return genlmsg_end(skb, hdr);
		genlmsg_end(skb, hdr);
		return 0;

		out_err:
		genlmsg_cancel(skb, hdr);

drivers/scsi/pmcraid.c

+1 −7

Original line number	Diff line number	Diff line
		@@ -1473,13 +1473,7 @@ static int pmcraid_notify_aen(
		}

		/* send genetlink multicast message to notify appplications */
		result = genlmsg_end(skb, msg_header);

		if (result < 0) {
		pmcraid_err("genlmsg_end failed\n");
		nlmsg_free(skb);
		return result;
		}
		genlmsg_end(skb, msg_header);

		result = genlmsg_multicast(&pmcraid_event_family, skb,
		0, 0, GFP_ATOMIC);