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

Re: [libvirt] [PATCH 2/2] Cleanup "/sys/class/net" usage




On 04/15/2015 05:51 AM, Michal Privoznik wrote:
> Throughout the code, we have several places need to construct a path
> somewhere in /sys/class/net/... They are not consistent and nearly
> each code piece invents its own way how to do it. So unify this by:
> 
> 1) use virNetDevSysfsFile() wherever possible
> 
> 2) At least use common macro SYSFS_NET_DIR declared in virnetdev.h at
>    the rest of places which can't go with 1)
> 
> Signed-off-by: Michal Privoznik <mprivozn redhat com>
> ---
>  src/Makefile.am                   |  4 ++--
>  src/parallels/parallels_network.c |  3 +--
>  src/util/virnetdev.c              |  5 ++---
>  src/util/virnetdev.h              |  2 ++
>  src/util/virnetdevbridge.c        |  1 -
>  src/util/virnetdevmacvlan.c       | 28 ++++++++++++++--------------
>  6 files changed, 21 insertions(+), 22 deletions(-)
> 

One more place w/ /sys/class/net: src/util/virnetdevveth.c
(cscope egrep search)

> diff --git a/src/Makefile.am b/src/Makefile.am
> index 91a4c17..3c9eac6 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1383,8 +1383,8 @@ noinst_LTLIBRARIES += libvirt_driver_parallels.la
>  libvirt_la_BUILT_LIBADD += libvirt_driver_parallels.la
>  libvirt_driver_parallels_la_CFLAGS = \
>  		-I$(srcdir)/conf $(AM_CFLAGS) \
> -		$(PARALLELS_SDK_CFLAGS)
> -libvirt_driver_parallels_la_LIBADD = $(PARALLELS_SDK_LIBS)
> +		$(PARALLELS_SDK_CFLAGS) $(LIBNL_CFLAGS)
> +libvirt_driver_parallels_la_LIBADD = $(PARALLELS_SDK_LIBS) $(LIBNL_LIBS)
>  libvirt_driver_parallels_la_SOURCES = $(PARALLELS_DRIVER_SOURCES)
>  endif WITH_PARALLELS
>  
> diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
> index 47f4886..47353a7 100644
> --- a/src/parallels/parallels_network.c
> +++ b/src/parallels/parallels_network.c
> @@ -28,6 +28,7 @@
>  #include "viralloc.h"
>  #include "virerror.h"
>  #include "virfile.h"
> +#include "virnetdev.h"
>  #include "md5.h"
>  #include "parallels_utils.h"
>  #include "virstring.h"
> @@ -39,8 +40,6 @@
>      virReportErrorHelper(VIR_FROM_TEST, VIR_ERR_OPERATION_FAILED, __FILE__,    \
>                       __FUNCTION__, __LINE__, _("Can't parse prlctl output"))
>  
> -#define SYSFS_NET_DIR "/sys/class/net"
> -
>  static int parallelsGetBridgedNetInfo(virNetworkDefPtr def, virJSONValuePtr jobj)
>  {
>      const char *ifname;

Technically - looks fine; however, does requiring the parallels driver
to link against libnl just to get a symbol seem reasonable? It's another
dependency right?

I would think it's 'safer' to change the call:

-    if (virAsprintf(&bridgeLink, "%s/%s/brport/bridge",
-                    SYSFS_NET_DIR, ifname) < 0)
+    if (virNetDevSysfsFile(&bridgeLink, ifname, "brport/bridge")) < 0)



> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 9ef75f2..a816e5d 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1612,14 +1612,13 @@ int virNetDevValidateConfig(const char *ifname ATTRIBUTE_UNUSED,
>  
>  
>  #ifdef __linux__
> -# define NET_SYSFS "/sys/class/net/"
>  
>  int
>  virNetDevSysfsFile(char **pf_sysfs_device_link, const char *ifname,
>                     const char *file)
>  {
>  
> -    if (virAsprintf(pf_sysfs_device_link, NET_SYSFS "%s/%s", ifname, file) < 0)
> +    if (virAsprintf(pf_sysfs_device_link, SYSFS_NET_DIR "%s/%s", ifname, file) < 0)
>          return -1;
>      return 0;
>  }
> @@ -1629,7 +1628,7 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname,
>                       const char *file)
>  {
>  
> -    if (virAsprintf(pf_sysfs_device_link, NET_SYSFS "%s/device/%s", ifname,
> +    if (virAsprintf(pf_sysfs_device_link, SYSFS_NET_DIR "%s/device/%s", ifname,
>                      file) < 0)
>          return -1;
>      return 0;
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 3535319..3e44d9f 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -219,6 +219,8 @@ int virNetDevSetRcvAllMulti(const char *ifname, bool receive)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>  int virNetDevGetRcvAllMulti(const char *ifname, bool *receive)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +
> +# define SYSFS_NET_DIR "/sys/class/net"
>  int virNetDevSysfsFile(char **pf_sysfs_device_link,
>                         const char *ifname,
>                         const char *file)
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index 6be8aa3..e601bdc 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -114,7 +114,6 @@ static int virNetDevBridgeCmd(const char *brname,
>  #endif
>  
>  #if defined(HAVE_STRUCT_IFREQ) && defined(__linux__)
> -# define SYSFS_NET_DIR "/sys/class/net"
>  /*
>   * Bridge parameters can be set via sysfs on newish kernels,
>   * or by  ioctl on older kernels. Perhaps we could just use
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 5fd2097..0eabee5 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -236,19 +236,15 @@ static
>  int virNetDevMacVLanTapOpen(const char *ifname,
>                              int retries)
>  {
> -    FILE *file;
> -    char path[64];
> +    int ret = -1;
> +    FILE *file = NULL;
> +    char *path;
>      int ifindex;
>      char tapname[50];
>      int tapfd;
>  
> -    if (snprintf(path, sizeof(path),
> -                 "/sys/class/net/%s/ifindex", ifname) >= sizeof(path)) {
> -        virReportSystemError(errno,
> -                             "%s",
> -                             _("buffer for ifindex path is too small"));
> +    if (virNetDevSysfsFile(&path, ifname, "ifindex") < 0)
>          return -1;
> -    }
>  
>      file = fopen(path, "r");
>  
> @@ -256,15 +252,14 @@ int virNetDevMacVLanTapOpen(const char *ifname,
>          virReportSystemError(errno,
>                               _("cannot open macvtap file %s to determine "
>                                 "interface index"), path);
> -        return -1;
> +        goto cleanup;
>      }
>  
>      if (fscanf(file, "%d", &ifindex) != 1) {
>          virReportSystemError(errno,
>                               "%s", _("cannot determine macvtap's tap device "
>                               "interface index"));
> -        VIR_FORCE_FCLOSE(file);
> -        return -1;
> +        goto cleanup;
>      }
>  
>      VIR_FORCE_FCLOSE(file);

There's a "if (snprintf...)" which does a return -1 that'll need to goto
cleanup (or of course a well placed VIR_FREE(path), but I do prefer the
cleanup option).

ACK with the adjustments

John

> @@ -288,12 +283,17 @@ int virNetDevMacVLanTapOpen(const char *ifname,
>          break;
>      }
>  
> -    if (tapfd < 0)
> +    if (tapfd < 0) {
>          virReportSystemError(errno,
>                               _("cannot open macvtap tap device %s"),
>                               tapname);
> -
> -    return tapfd;
> +        goto cleanup;
> +    }
> +    ret = tapfd;
> + cleanup:
> +    VIR_FREE(path);
> +    VIR_FORCE_FCLOSE(file);
> +    return ret;
>  }
>  
>  
> 


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