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

[Michal Privoznik]

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 at 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