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(a)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;
}