[libvirt] [PATCH 3/4] util: use nlmsg_find_attr() instead of an open-coded loop
Erik Skultety
eskultet at redhat.com
Fri Jan 11 07:28:07 UTC 2019
On Thu, Jan 10, 2019 at 11:42:08AM -0500, Laine Stump wrote:
> On 1/10/19 10:38 AM, Erik Skultety wrote:
> > On Thu, Jan 10, 2019 at 09:34:35AM -0500, Laine Stump wrote:
> > > 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 at laine.org>
> > > > > ---
> > > > > 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.
> > In which case it qualifies as a warning, I am not entirely convinced we want
> > that information to be lost in the flood of DEBUG messages.
>
>
> If virNetDevGetName() fails to convert, it logs an error. Of course then you
> just end up with an error message saying that virNetDevGetName() failed to
> convert ifindex "blah" to a name, and no context about what was happening
> when that occurred. I still think that additionally having the debug log
> available regardless of whether or not virNetDevGetName() fails could be
> useful some day, and it's not taking any extra effort to do so, but if it
> really rubs you the wrong way, then I'll adjust it :-)
It's not a show stopper, please push as is.
More information about the libvir-list
mailing list