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@xxxxxxxxxx>
> ---
> 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;
> }
>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list