Re: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
[...]
> +
> +static int
> +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> + void *opaque)
> +{
> + struct rtmsg *rtmsg = NLMSG_DATA(resp);
> + int accept_ra = -1;
> + struct rtattr *rta;
> + char *ifname = NULL;
> + struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> + int ret = 0;
> + int len = RTM_PAYLOAD(resp);
> + int oif = -1;
> +
> + /* Ignore messages other than route ones */
> + if (resp->nlmsg_type != RTM_NEWROUTE)
> + return ret;
> +
> + /* Extract a few attributes */
> + for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> + switch (rta->rta_type) {
> + case RTA_OIF:
> + oif = *(int *)RTA_DATA(rta);
> +
> + if (!(ifname = virNetDevGetName(oif)))
> + goto error;
> + break;
Did you really mean to break from the for loop if ifname is set? This
breaks only from the switch/case. Of course Coverity doesn't know much
more than you'd be going back to the top of the for loop and could
overwrite ifname again. It proclaims a resource leak...
In my dev version I was also getting the RTA_DST attribute to print some
debugging message that I removed in the submitted version. The break was
here between the switch cases.
In the current case I don't really care if we break out of the loop or not.
As there aren't that many attributes in an rtnetlink message to loop over,
breaking wouldn't gain a lot of cycles.
What I don't get though is what this break is actually doing? isn't it
for the switch case even though there is no other case after it?
--
Cedric
John
> + }
> + }
> +
> + /* No need to do anything else for non RA routes */
> + if (rtmsg->rtm_protocol != RTPROT_RA)
> + goto cleanup;
> +
> + data->hasRARoutes = true;
> +
> + /* Check the accept_ra value for the interface */
> + accept_ra = virNetDevIPGetAcceptRA(ifname);
> + VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname,
accept_ra);
> +
> + if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices,
data->ndevices, ifname) < 0)
> + goto error;
> +
> + cleanup:
> + VIR_FREE(ifname);
> + return ret;
> +
> + error:
> + ret = -1;
> + goto cleanup;
> +}