[libvirt] [PATCH RESEND] Interface: return appropriate status for bridge device
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 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'. This patch fixes the issue by counting slave devices for bridge device. Signed-off-by: Lin Ma <lma@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"); + 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) + goto cleanup; + member_count = scandir(member_path, &member_list, + udevBridgeScanDirFilter, alphasort); + if (member_count > 0) + status = 1; + for (i = 0; i < member_count; i++) + VIR_FREE(member_list[i]); + VIR_FREE(member_list); + VIR_FREE(member_path); + } + } + udev_device_unref(dev); cleanup: -- 2.9.2
[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@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:
On 08/02/2017 05:09 PM, John Ferlan wrote:
[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!
Actually it's a problem that the udev-based interface driver lists transient interfaces like virbr0 *at all*. The netcf backend for the interface driver only shows interfaces that are in the host's persistent system network config, it doesn't list anything created by libvirt's virtual network drivers - the two spaces are supposed to be separate from each other. This was one of the problems I had with the udev backend for the interface driver back when it was written - it isn't providing the same list of devices that the netcf backend does, and that means that consumers like virt-manager will show different behavior on different distros (e.g. it will either erroneously show virbr0 as a bridge that you might want to connect a guest to directly, or show some other strange transient interface as a potential connection for macvtap). The basic problem, of course, is that netcf looks to the system network config to get the canonical list of network interfaces' and their persistent configuration, while the udev backend looks at the current status of network interfaces via udev and sysfs, paying no attention to persistent config. In the end, if you really care about the online status of virbr0 as provided by iface-dumpxml, then you're using the virInterface APIs in a way that will behave differently on different distros, and I don't know how good of an idea that is. If, on the other hand, you're seeing a problem with bridge devices that are in the host system's persistent network config (e.g. a bridge that's attached to a physical ethernet), then I guess I can see the utility of getting this right (although in that case, it would be following the operstate of the physical ethernet anyway, wouldn't it?) Beyond that, because my position has always been that distros should either use netcf, or at least use a backend with the same semantics as netcf rather than something that's just providing a "similar approximation" of netcf semantics, I don't really have much to say about the internal details. This is one of those cases where "if it works, and doesn't make the already-objectionable code any more objectionable, then sure, whatever". Sorry if I'm not more enthusiastic about this code (not your patches, but the code that you're applying the patches to). I've just never been a fan.
This patch fixes the issue by counting slave devices for bridge device.
Signed-off-by: Lin Ma <lma@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:
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
John Ferlan -
Laine Stump -
Lin Ma