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

Re: [libvirt] [PATCH v2 1/2] Make virNetDevSetupControl() public.



On 05/25/2013 09:24 AM, Roman Bogorodskiy wrote:
> This method is useful not only in virnetdev.c.
> ---
>  src/libvirt_private.syms         |  1 +
>  src/util/virnetdev.c             | 15 +++++++++++++--
>  src/util/virnetdev.h             | 11 +++++++++++
>  src/util/virnetdevmacvlan.c      |  2 +-
>  src/util/virnetdevvportprofile.c |  2 +-
>  5 files changed, 27 insertions(+), 4 deletions(-)
> 

You don't have 'cppi' installed, or 'make syntax-check' would have
pointed out these nits:

preprocessor_indentation
cppi: src/util/virnetdev.h: line 33: not properly indented
cppi: src/util/virnetdev.h: line 37: not properly indented
cppi: src/util/virnetdev.h: line 40: not properly indented
maint.mk: incorrect preprocessor indentation

> +#else
> +int
> +virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED,
> +                      void *ifr ATTRIBUTE_UNUSED)
> +{
> +    virReportSystemError(ENOSYS,
> +                         _("%s is not supported on this platform"),
> +                         __func__);

I liked Dan's suggestion for how to word this:
>> How about
>> 
>> "Network device configuration is not supported on this platform"


> +#ifdef HAVE_STRUCT_IFREQ
> +int virNetDevSetupControl(const char *ifname,
> +                          struct ifreq *ifr)
> +    ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +#else
> +int virNetDevSetupControl(const char *ifname,
> +                          void *ifr);
> +#endif

I'm not sure I'm a fan of changing the compile-time attributes based on
whether the struct exists; I'm proposing an alternate solution below.

> +++ b/src/util/virnetdevmacvlan.c
> @@ -49,7 +49,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST,
>  # include <sys/socket.h>
>  # include <sys/ioctl.h>
>  
> -# include <linux/if.h>
> +# include <net/if.h>
>  # include <linux/if_tun.h>

Independently useful.  I might split your patch into two, and apply the
<net/if.h> usage right away while awaiting feedback on my proposed tweaks.

diff --git i/src/util/virnetdev.c w/src/util/virnetdev.c
index f658c6d..1316bc4 100644
--- i/src/util/virnetdev.c
+++ w/src/util/virnetdev.c
@@ -99,9 +99,9 @@ int
 virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED,
                       void *ifr ATTRIBUTE_UNUSED)
 {
-    virReportSystemError(ENOSYS,
-                         _("%s is not supported on this platform"),
-                         __func__);
+    virReportSystemError(ENOSYS, "%s",
+                         _("Network device configuration is not supported "
+                           "on this platform"));
     return -1;
 }
 #endif
diff --git i/src/util/virnetdev.h w/src/util/virnetdev.h
index 97369c1..3faff48 100644
--- i/src/util/virnetdev.h
+++ w/src/util/virnetdev.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007-2012 Red Hat, Inc.
+ * Copyright (C) 2007-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -30,14 +30,15 @@
 # include "virmacaddr.h"
 # include "virpci.h"

-#ifdef HAVE_STRUCT_IFREQ
+# ifdef HAVE_STRUCT_IFREQ
+typedef struct ifreq virIfreq;
+# else
+struct virIfreq { };
+# endif
+
 int virNetDevSetupControl(const char *ifname,
-                          struct ifreq *ifr)
+                          virIfreq *ifr)
     ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-#else
-int virNetDevSetupControl(const char *ifname,
-                          void *ifr);
-#endif

 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]