[libvirt] [PATCH RESEND] Interface: return appropriate status for bridge device

[John Ferlan]

[pardon the top-post comment]
Doing some (personal) mail box cleaning and found this "older" patch...
Sorry that this has sat unattended (and probably now forgotten or given
up on) for so long. Suppose it's an assumption that someone like Laine
who understand bonds/bridges better than others would pick it up...

Although, it's not my space, I'll provide some feedback in general and
if you want to rework, send again - then hopefully someone more in the
know about "expectations" can look at it...

On 02/13/2017 05:06 AM, Lin Ma wrote:
> In function udevInterfaceIsActive, The current design relies on the sys
> attribute 'operstate' to determine whether the interface is active.
> 
> For the device node of virtual network(say virbr0), There is already a
> dummy tap device to maintain a fixed mac on it, So it's available and
> its status should be considered as active. But if no anyelse tap device

anyelse needs to be adjusted.

> connects to it, The value of operstate of virbr0 in sysfs is 'down', So
> udevInterfaceIsActive returns 0, It causes 'virsh iface-list --all' to
> report the status of virbr0 as 'inactive'.

Personally, I'm not sure if this is a feature or a problem, hopefully
Laine can elaborate!

> 
> This patch fixes the issue by counting slave devices for bridge device.
> 
> Signed-off-by: Lin Ma <lma at suse.com>
> ---
>  src/interface/interface_backend_udev.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
> index 5d0fc64..9ac4674 100644
> --- a/src/interface/interface_backend_udev.c
> +++ b/src/interface/interface_backend_udev.c
> @@ -1127,6 +1127,11 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
>      struct udev_device *dev;
>      virInterfaceDefPtr def = NULL;
>      int status = -1;
> +    struct dirent **member_list = NULL;
> +    const char *devtype;
> +    int member_count = 0;
> +    char *member_path;
> +    size_t i;
>  
>      dev = udev_device_new_from_subsystem_sysname(udev, "net",
>                                                   ifinfo->name);
> @@ -1146,6 +1151,23 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
>      /* Check if it's active or not */
>      status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
>  

The following hunk should be in a "helper" - that is create another
function...

    if (!status) {
        devtype = udev_device_get_devtype(dev);
        if (STREQ_NULLABLE(devtype, "bridge"))
            status = newProperlyNamedHelper(arg1, arg2);
    }

Although it's almost too bad the bridge/bond code processing scandir
list output couldn't be combined in some way...  It's made tricky by
using bridge vs. bond, but I think the code has a lot of similarities
and allocation of specific fields and list traversal could probably be
handled by some callback type function. Not required for another patch,
but might be interesting to try.

> +    if (!status) {
> +        devtype = udev_device_get_devtype(dev);
> +        if (STREQ_NULLABLE(devtype, "bridge")) {
> +            if (virAsprintf(&member_path, "%s/%s",
> +                        udev_device_get_syspath(dev), "brif") < 0)

This should have been properly aligned (udev_* under &member_path).
Although I assume this is essentially a copy from udevGetIfaceDefBridge.

> +                goto cleanup;
> +            member_count = scandir(member_path, &member_list,
> +                    udevBridgeScanDirFilter, alphasort);

Similarly w/r/t alignment, but udevB* under member_path.

> +            if (member_count > 0)
> +                status = 1;
> +            for (i = 0; i < member_count; i++)
> +                VIR_FREE(member_list[i]);
> +            VIR_FREE(member_list);

This I believe could be replaced by virStringListFree (and similar for
other consumers using scandir results)


John

> +            VIR_FREE(member_path);
> +        }
> +    }
> +>      udev_device_unref(dev);
>  
>   cleanup:
>