On 1/10/19 9:09 AM, Erik Skultety wrote:
On Wed, Jan 09, 2019 at 12:43:14PM -0500, Laine Stump wrote:
This is about the same number of code lines, but is simpler, and more
consistent with what will be added to check another attribute in a
coming patch.
As a side effect, it
Resolves: https://bugzilla.redhat.com/1583131
Signed-off-by: Laine Stump <laine@xxxxxxxxx>
---
src/util/virnetdevip.c | 53 ++++++++++++++++++------------------------
1 file changed, 23 insertions(+), 30 deletions(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 72048e4b45..c032ecacfc 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -529,49 +529,42 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
void *opaque)
{
struct rtmsg *rtmsg = NLMSG_DATA(resp);
- int accept_ra = -1;
- struct rtattr *rta;
struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
- int len = RTM_PAYLOAD(resp);
- int oif = -1;
+ struct rtattr *rta_attr;
+ int accept_ra = -1;
+ int ifindex = -1;
VIR_AUTOFREE(char *) ifname = NULL;
/* Ignore messages other than route ones */
if (resp->nlmsg_type != RTM_NEWROUTE)
return 0;
- /* Extract a device ID attribute */
- VIR_WARNINGS_NO_CAST_ALIGN
- for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
- VIR_WARNINGS_RESET
- if (rta->rta_type == RTA_OIF) {
- oif = *(int *)RTA_DATA(rta);
-
- /* Should never happen: netlink message would be broken */
- if (ifname) {
- VIR_AUTOFREE(char *) ifname2 = virNetDevGetName(oif);
- VIR_WARN("Single route has unexpected 2nd interface "
- "- '%s' and '%s'", ifname, ifname2);
- break;
- }
-
- if (!(ifname = virNetDevGetName(oif)))
- return -1;
- }
- }
-
/* No need to do anything else for non RA routes */
if (rtmsg->rtm_protocol != RTPROT_RA)
return 0;
- data->hasRARoutes = true;
+ rta_attr = (struct rtattr *)nlmsg_find_attr(resp, sizeof(struct rtmsg), RTA_OIF);
+ if (rta_attr) {
+ /* This is a single path route, with interface used to reach
+ * nexthop in the RTA_OIF attribute.
+ */
+ ifindex = *(int *)RTA_DATA(rta_attr);
+ ifname = virNetDevGetName(ifindex);
- /* 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 (ifname)
I'd put
if (!ifname)
return -1;
^ here instead, since having (null) in the DEBUG output doesn't really help
anyone and...
I disagree with that. Having a null ifname means that the ifindex sent
as RTA_OIF couldn't be resolved to a proper name. Allowing the code to
make it through to the VIR_DEBUG and print out the offending ifindex
(along with "(null)") will give us more info to further investigate.
+ accept_ra = virNetDevIPGetAcceptRA(ifname);
- if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
- return -1;
+ VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d",
+ ifname, ifindex, accept_ra);
+
+ if (!ifname ||
... we'd return failure here anyway.
+ (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)) {
+ return -1;
+ }
+
+ data->hasRARoutes = true;
+ return 0;
I think ^this return should be part of the next patch where it IMHO makes more
sense.
I included it here because the code is still correct with it in, and it
makes the next patch more self-contained (the only code added is the
code directly related to checking the nexthop interfaces).
(truthfully, this started out as a single patch, and I split it into
parts to make it easier to review. I'd be just as happy to turn it back
into a single patch :-)
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
