[libvirt] [PATCH 0/2] Couple of virnetdevtest cleanups

*** BLURB HERE *** Michal Privoznik (2): tests: Add virnetdevtestdata to EXTRA_DIST Cleanup "/sys/class/net" usage 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 ++++++++++++++-------------- tests/Makefile.am | 1 + 7 files changed, 22 insertions(+), 22 deletions(-) -- 2.0.5

In one of my previous commits (49ed6cff9) I've introduced a test among with some files stored under virnetdevtestdata folder. While this works perfectly within a git tree, the folder was not getting into .tar.gz and therefore the dist-check would fail. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 9ebedc3..5f88b18 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -133,6 +133,7 @@ EXTRA_DIST = \ virsh-uriprecedence \ vircgroupdata \ virfiledata \ + virnetdevtestdata \ virpcitestdata \ virscsidata \ virusbtestdata \ -- 2.0.5

On 04/15/2015 05:51 AM, Michal Privoznik wrote:
In one of my previous commits (49ed6cff9) I've introduced a test among with some files stored under virnetdevtestdata folder. While this works perfectly within a git tree, the folder was not getting into .tar.gz and therefore the dist-check would fail.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/Makefile.am | 1 + 1 file changed, 1 insertion(+)
ACK John

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

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; }

On 15.04.2015 15:07, John Ferlan wrote:
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)
That would not help much. The Makefile.am change (dragging in LIBNL_CFLAGS) is basically due to include of virnetdev.h, which includes virnetlink.h, which includes (conditionally) netlink/msg.h. I've fixed the nits you pointed out and pushed. Thanks! Michal
participants (2)
-
John Ferlan
-
Michal Privoznik