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

Laine Stump laine at laine.org
Tue Feb 3 14:48:01 UTC 2015


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 at redhat.com>
> ---
>  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__ */




More information about the libvir-list mailing list