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

Re: [libvirt] [PATCH v3 2/4] Make virNetDevSetupControl() public.



On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote:
> ---
>  src/libvirt_private.syms | 1 +
>  src/util/virnetdev.c     | 6 ++++--
>  src/util/virnetdev.h     | 6 ++++++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c589236..8ad4634 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1518,6 +1518,7 @@ virNetDevSetMTUFromDevice;
>  virNetDevSetName;
>  virNetDevSetNamespace;
>  virNetDevSetOnline;
> +virNetDevSetupControl;
>  virNetDevValidateConfig;
>  
>  
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 503db9d..9ee84c3 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -78,8 +78,9 @@ static int virNetDevSetupControlFull(const char *ifname,
>  }
>  
>  
> -static int virNetDevSetupControl(const char *ifname,
> -                                 struct ifreq *ifr)
> +int
> +virNetDevSetupControl(const char *ifname,
> +                          struct ifreq *ifr)

Indentation is off on the second line, now (the first s of 'struct'
should be directly below the c of 'const').

>  {
>  #if defined(__FreeBSD__)
>      return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM);
> @@ -87,6 +88,7 @@ static int virNetDevSetupControl(const char *ifname,
>      return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM);
>  #endif
>  }
> +#else /* !(defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)) */
>  #endif

What good is it to add an #else with no body?  If you are trying to mark
the end of an #if, it might be better to do:

#endif /* defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__) */

Meanwhile, this particular change fits better in patch 1/4, and needs to
account for my suggestions of doing a feature probe rather than an OS probe.

> +++ b/src/util/virnetdev.h
> @@ -28,6 +28,12 @@
>  # include "virmacaddr.h"
>  # include "virpci.h"
>  
> +# include <net/if.h>

<net/if.h> is a standard header (and therefore guaranteed by gnulib), but...

> +
> +int virNetDevSetupControl(const char *ifname,
> +                          struct ifreq *ifr)

struct ifreq is a non-standard type, and therefore this will cause
compilation errors on platforms that lack this struct (hello mingw).  We
have two choices: make a new typedef virIfreqPtr that maps to 'struct
ifreq *' on Linux and FreeBSD, and to 'void *' on other platforms; or
make this prototype be conditional on whether HAVE_STRUCT_IFREQ (or
whatever the right name is) was determined at configure time.

> +    ATTRIBUTE_RETURN_CHECK;

It is probably also worth using ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2).

> +
>  int virNetDevExists(const char *brname)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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