Re: [libvirt PATCH] conf: allow for a partial match when searching for an <interface>

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 22, 2021 at 05:30:34PM -0400, Laine Stump wrote:
> Commit 114e3b42 modified virDomainNetFindIdx() to compare the alias
> name of the <interface> being matched (in addition to already-existing
> match of MAC address and PCI/CCW address). This was done in response
> to https://bugzilla.redhat.com/1926190 which complained that it wasn't
> possible to unplug an interface that had the same MAC address as
> another <interface> (which is the way interfaces are setup when using
> <teaming> - the "team" is identified in the guest virtio-net driver by
> looking for another interface with the same MAC as the virtio-net
> device) - the reporter of that bug did not have PCI addresses of the
> devices easily available when attempting to unplug one of the two
> devices, and so needed some other way to disambiguate the two
> interfaces with matching MACs.
> 
> Unfortunately, this caused a regression which was caught during QE
> testing - in the past it had been possible to use
> virDomainUpdateDevice (which also calls virDomainNetFindIdx()) to
> modify the alias name of an existing interface - with the change in
> commit 114e3b42 this was no longer possible (since we would try to
> match the new alias, which would of course always fail).
> 
> The solution to this regression is to allow mismatched alias names
> when calling virDomainNetFindIdx() for purposes of updating a device
> (indicated by the new bool argument "partialMatch"). When calling for
> unplug the old behavior is maintained - in that case the alias name
> must still match.

I find this seriously dubious. Essentially this is saying that alias
is the unique identifier used to match, except when it isn't used to
match. Renaming the unique identifier is a questionable action, and
I'd claim that fact that it worked previously is an accident rather
than by design.

> 
> Because we need to keep track of potentially multiple "partial"
> matches so that we can go back later and try to disambiguate when
> necessary, I needed an array to hold the indexes of all the matches
> during the "first round". There is guidance in glib-adoption.rst
> saying that new libvirt code should prefer GArray/GPtrArray. A couple
> of adventurous souls have used GPtrArray, but so far nobody has used
> GArray, and I decided this was a good chance to try that out. It went
> okay.
> 
> Reported-by: Yalan Zhang <yalzhang@xxxxxxxxxx>
> Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> ---
> 
> I realized while writing this patch that an update-device test case in
> qemuhotplugtest would have caught this regression and so I probably
> should add such a test case to prevent it happening again, but the
> testing harness for update-device was only ever made to work for
> graphics devices, meaning there's some unknown amount of investigating
> and rejiggering that needs to be done to make such a test work (a
> quick attempt failed). Since someone is waiting on the fix for this
> regression, I'm hoping that I can get a reprieve on the "add a test
> case to catch thre regression" part that should accompany a bugfix
> like this in exchange for a promise that I will start looking into
> that immediately after I get this pushed (and then backported so
> testing of the bugzilla above can be completed)
> 
> 
>  src/conf/domain_conf.c   | 207 +++++++++++++++++++++++++--------------
>  src/conf/domain_conf.h   |   2 +-
>  src/libxl/libxl_driver.c |   4 +-
>  src/lxc/lxc_driver.c     |   6 +-
>  src/qemu/qemu_driver.c   |   6 +-
>  src/qemu/qemu_hotplug.c  |   4 +-
>  6 files changed, 145 insertions(+), 84 deletions(-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux