Re: [PATCH] configure: Fix devlink detection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2017-09-19 at 12:29 +0200, Ján Tomko wrote:
> > +    AC_CHECK_DECLS([DEVLINK_GENL_VERSION, DEVLINK_GENL_NAME, DEVLINK_ATTR_MAX, DEVLINK_ATTR_BUS_NAME, DEVLINK_ATTR_DEV_NAME,
> 
> All these constants were introduced by commit bfcd3a4661 along with the
> devlink infractructure. Having any of them is a sufficient witness for
> HAVE_DECL_DEVLINK and pointless to check here.

I did not dig into the Linux code, so I was not aware of that, but
since that's the case we can definitely reduce the list to a single
item.

> As for these two:
> DEVLINK_ATTR_ESWITCH_MODE, DEVLINK_ESWITCH_MODE_SWITCHDEV],
> 
> we only need to check for ESWITCH_MODE. If there are any modes to
> check for, SWITCHDEV should be one of the known ones.

Wouldn't it make more sense to check for _MODE_SWITCHDEV instead?
Surely if that is available, then _MODE will be too.

> > @@ -3245,7 +3245,12 @@ virNetDevSwitchdevFeature(const char *ifname,
> >     if (!(gmsgh = virNetDevPutExtraHeader(nlmsg_hdr(nl_msg), sizeof(struct genlmsghdr))))
> >         goto cleanup;
> > 
> > +#if HAVE_DEVLINK_CMD_ESWITCH_GET
> >     gmsgh->cmd = DEVLINK_CMD_ESWITCH_GET;
> > +#elif HAVE_DEVLINK_CMD_ESWITCH_MODE_GET
> > +    gmsgh->cmd = DEVLINK_CMD_ESWITCH_MODE_GET;
> 
> We really don't need to litter the code with obsolete constants.

I would totally agree if we were trying to get around the name
change by using the legacy name unconditionally, but my
implementation ensures we're only using the legacy name when the
new one is not available.

So in no situation we're using deprecated symbols, and we can
still compile the devlink code on systems that don't have the
new one such as RHEL 7. Isn't that worth the bit of uglyness the
code above introduces?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux