12:43 p.m.
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list