On 02/03/2015 03:48 PM, Laine Stump wrote:
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@xxxxxxxxxx> --- 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?
Sure, I can do that too. I was wandering whether to also update that and I told myself that it would be probably better to unify that formatting globally for the whole libvirt code.
- 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).
Actually MinGW was unhappy about this. Thanks for review. I'll update the commit and push it. Pavel
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__ */
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list