Re: [libvirt] [PATCH v2 2/4] virnetdev: Introduce virNetDevGetLinkInfo
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(a)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