Re: [libvirt] [PATCH v2 1/2] virnetdev: fix some issues found by coverity and mingw builds

On 02/03/2015 08:27 AM, Pavel Hrdina wrote: > Commit e562a61a introduced new function to get/set interface state but > there was misuse of ATTRIBUTE_NONNULL on non-pointer attributes and also > we need to wrap that functions by #ifdef to not break mingw build. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > src/util/virnetdev.c | 36 ++++++++++++++++++++++++------------ > src/util/virnetdev.h | 6 +++--- > 2 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c > index 4be6265..315c431 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -66,6 +66,18 @@ VIR_LOG_INIT("util.netdev"); > #define VIR_MCAST_TOKEN_DELIMS " \n" > #define VIR_MCAST_ADDR_LEN (VIR_MAC_HEXLEN + 1) > > +#if defined(SIOCSIFFLAGS) && defined(HAVE_STRUCT_IFREQ) > +# define VIR_IFF_UP IFF_UP > +# define VIR_IFF_PROMISC IFF_PROMISC > +# define VIR_IFF_MULTICAST IFF_MULTICAST > +# define VIR_IFF_ALLMULTI IFF_ALLMULTI > +#else > +# define VIR_IFF_UP 0 > +# define VIR_IFF_PROMISC 0 > +# define VIR_IFF_MULTICAST 0 > +# define VIR_IFF_ALLMULTI 0 > +#endif > + Sigh. I should have noticed this when I sent the patch making virNetDev[GS]etIFFlag static - I even mentioned that I wanted to *avoid* the need to define our own private versions of the flags, but was too dense to realize that we *already* needed to do that :-/ One fine pedantic point - if it were to every happen that a platform had SIOCGIFFLAGS but not SIOCSIFFLAGS, the incorrect values would be defined for VIR_IFF_*. Since that is *never* going to happen, and since the alternative of using "#if defined(IFF_UP)..." could lead to silently misdefined values if some platform only had enum values for IFF_* but no #defines, I think the way you've done it is the best option. > typedef enum { > VIR_MCAST_TYPE_INDEX_TOKEN, > VIR_MCAST_TYPE_NAME_TOKEN, > @@ -677,7 +689,7 @@ int virNetDevSetOnline(const char *ifname, > bool online) > { > > - return virNetDevSetIFFlag(ifname, IFF_UP, online); > + return virNetDevSetIFFlag(ifname, VIR_IFF_UP, online); > } > > /** > @@ -694,7 +706,7 @@ int virNetDevSetOnline(const char *ifname, > int virNetDevSetPromiscuous(const char *ifname, > bool promiscuous) > { As long as you're touching all of these very short functions, can you also reformat the first lines of all of them to have the return type on a separate line from the function name?

> - return virNetDevSetIFFlag(ifname, IFF_PROMISC, promiscuous); > + return virNetDevSetIFFlag(ifname, VIR_IFF_PROMISC, promiscuous); > } > > /** > @@ -712,7 +724,7 @@ int virNetDevSetPromiscuous(const char *ifname, > int virNetDevSetRcvMulti(const char *ifname, > bool receive) > { > - return virNetDevSetIFFlag(ifname, IFF_MULTICAST, receive); > + return virNetDevSetIFFlag(ifname, VIR_IFF_MULTICAST, receive); > } > > /** > @@ -728,7 +740,7 @@ int virNetDevSetRcvMulti(const char *ifname, > int virNetDevSetRcvAllMulti(const char *ifname, > bool receive) > { > - return virNetDevSetIFFlag(ifname, IFF_ALLMULTI, receive); > + return virNetDevSetIFFlag(ifname, VIR_IFF_ALLMULTI, receive); > } > > > @@ -783,7 +795,7 @@ virNetDevGetIFFlag(const char *ifname, > int virNetDevGetOnline(const char *ifname, > bool *online) > { > - return virNetDevGetIFFlag(ifname, IFF_UP, online); > + return virNetDevGetIFFlag(ifname, VIR_IFF_UP, online); > } > > /** > @@ -797,9 +809,9 @@ int virNetDevGetOnline(const char *ifname, > * Returns 0 in case of success or an errno code in case of failure. > */ > int virNetDevGetPromiscuous(const char *ifname, > - bool *promiscuous) > + bool *promiscuous) > { > - return virNetDevGetIFFlag(ifname, IFF_PROMISC, promiscuous); > + return virNetDevGetIFFlag(ifname, VIR_IFF_PROMISC, promiscuous); > } > > /** > @@ -813,9 +825,9 @@ int virNetDevGetPromiscuous(const char *ifname, > * Returns 0 in case of success or -1 on error. > */ > int virNetDevGetRcvMulti(const char *ifname, > - bool *receive) > + bool *receive) > { > - return virNetDevGetIFFlag(ifname, IFF_MULTICAST, receive); > + return virNetDevGetIFFlag(ifname, VIR_IFF_MULTICAST, receive); > } > > /** > @@ -829,9 +841,9 @@ int virNetDevGetRcvMulti(const char *ifname, > * Returns 0 in case of success or -1 on error. > */ > int virNetDevGetRcvAllMulti(const char *ifname, > - bool *receive) > + bool *receive) > { > - return virNetDevGetIFFlag(ifname, IFF_ALLMULTI, receive); > + return virNetDevGetIFFlag(ifname, VIR_IFF_ALLMULTI, receive); > } > > > @@ -2668,7 +2680,7 @@ int virNetDevGetRxFilter(const char *ifname, > virNetDevRxFilterPtr *filter) > { > int ret = -1; > - bool receive; > + bool receive = false; > virNetDevRxFilterPtr fil = virNetDevRxFilterNew(); I assume that the above was done to silence a coverity error, since receive is definitely never used before it is set. Assuming that, ACK (with the first lines of all the functions split, as requested above).

> > if (!fil) > diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h > index 6e8372f..de8b480 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -201,17 +201,17 @@ int virNetDevDelMulti(const char *ifname, > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > > int virNetDevSetPromiscuous(const char *ifname, bool promiscuous) > - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > int virNetDevGetPromiscuous(const char *ifname, bool *promiscuous) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > > int virNetDevSetRcvMulti(const char *ifname, bool receive) > - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > int virNetDevGetRcvMulti(const char *ifname, bool *receive) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > > int virNetDevSetRcvAllMulti(const char *ifname, bool receive) > - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > int virNetDevGetRcvAllMulti(const char *ifname, bool *receive) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; > #endif /* __VIR_NETDEV_H__ */