[libvirt] [PATCH v2 2/4] virnetdev: Introduce virNetDevGetLinkInfo

[Michal Privoznik]

On 11.06.2014 03:13, John Ferlan wrote:
>
>
> On 06/05/2014 11:39 AM, Michal Privoznik wrote:
>> The purpose of this function is to fetch link state
>> and link speed for given NIC name from the SYSFS.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/libvirt_private.syms |   1 +
>>   src/util/virnetdev.c     | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>>   src/util/virnetdev.h     |   6 +++
>>   3 files changed, 111 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 394c086..6d08ee9 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1540,6 +1540,7 @@ virNetDevClearIPv4Address;
>>   virNetDevExists;
>>   virNetDevGetIndex;
>>   virNetDevGetIPv4Address;
>> +virNetDevGetLinkInfo;
>>   virNetDevGetMAC;
>>   virNetDevGetMTU;
>>   virNetDevGetPhysicalFunction;
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 3a60cf7..9d2c0d2 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1832,3 +1832,107 @@ virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED,
>>   }
>>
>>   #endif /* defined(__linux__) && defined(HAVE_LIBNL) */
>> +
>> +#ifdef __linux__
>> +# define SYSFS_PREFIX "/sys/class/net/"
>
> There is a 'NET_SYSFS' definition earlier to the same path - seems it
> could/should be reused...
>
> NIT: Your definition will create a double slash below for operstate and
> speed... Whether that's desired or not is up to you...  There's a mix of
> usage within libvirt sources.

In fact, I've substituted both virAsprintf()s with virNetDevSysfsFile() 
which does exactly what I need.

>
>> +int
>> +virNetDevGetLinkInfo(const char *ifname,
>> +                     virInterfaceState *state,
>> +                     unsigned int *speed)
>
> Could there ever be any other stats/data to fetch?  Perhaps consider
> passing a virInterfaceLinkPtr instead and then reference state/speed off
> of it.  Then in the future you don't have to keep adding params to the
> API.  None of the current callers pass NULL for either parameter.

Right. The API design is a leftover from previous patch set where I was 
implementing netcf backend too. Fixed.

>
> I also wonder if we really want to fail out if we something goes wrong.
> It's not like we're affecting the state/speed of the device if something
> did go wrong - we're just outputting data if it's there. I guess I'm
> thinking more about the callers, but that's a design decision you have
> to make and live with. Considering the comment below regarding the bug
> with speed - one wonders what else lurks in former formats that the
> following code can error out on.

I'd say error out in this function and let caller decide whether he 
wants to ignore error or not. If I make this function fault tolerant, 
caller loses that opportunity.

>
>> +{
>> +    int ret = -1;
>> +    char *path = NULL;
>> +    char *buf = NULL;
>> +    char *tmp;
>> +    int tmp_state;
>
> s/int/virInterfaceState/
>

In fact this is intentional. Remember Eric's TypeFromString() patches? 
The problem is, one can't be sure if compiler decides on using unsigned 
or signed integer to represent an enum. If it decides to use an unsigned 
int (IIRC that's the case of older gcc) then comparison a few lines 
below will never be true. That's why I'm introducing this dummy 
variable. I could as well assign the result to the destination variable 
directly. BTW: old compilers are the reason why I'm crippling some 
variable names, e.g. 'virInterfaceLinkPtr lnk' vs. 'virInterfaceLinkPtr 
link' as older gcc fails to see 'link' is a variable not the 'link()' 
function.

>
> Seems we should be able to set up a dummy /sys/class/net environment in
> order to test the new API's... Something I attempted with my recently
> posted scsi_host changes.

Yeah, unit testing is certainly in place. But honestly, I thought it 
would blow the size of this patches up. Again, something that a follow 
up patch will do.

Michal