[libvirt PATCH] conf: allow for a partial match when searching for an <interface>
Michal Privoznik
mprivozn at redhat.com
Tue Mar 23 12:44:49 UTC 2021
On 3/22/21 10:30 PM, 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.
>
> 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 at redhat.com>
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
>
> 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(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 95602ae57e..f071bf93d0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16871,109 +16871,170 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
> * virDomainNetFindIdx:
> * @def: domain definition
> * @net: interface definition
> + * @matchPartial: true if mismatched alias is still a match
> *
> - * Lookup domain's network interface based on passed @net
> - * definition. If @net's MAC address was auto generated,
> - * the MAC comparison is ignored.
> + * Find a network interface from the domain's nets list by looking for
> + * a match to @net. The following attributes are compared when
> + * specified in @net: MAC address, PCI/CCW address, and alias name.
> + *
> + * If matchPartial is true, then an entry with mismatched alias name
> + * still counts as a match (as long as everything else that was
> + * specified matches).
> + *
> + * If @net matches multiple items in nets, that is an error.
> *
> * Return: index of match if unique match found,
> * -1 otherwise and an error is logged.
> */
> int
> -virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
> +virDomainNetFindIdx(virDomainDefPtr def,
> + virDomainNetDefPtr net,
> + bool matchPartial)
> {
> + guint gi;
> size_t i;
> - int matchidx = -1;
> + g_autoptr(GArray) matches
> + = g_array_sized_new(false, false, sizeof(size_t), def->nnets);
How about breaking the line later? Somewhere in the argument list just
like ...
> char mac[VIR_MAC_STRING_BUFLEN];
> bool MACAddrSpecified = !net->mac_generated;
> bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info,
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
> bool CCWAddrSpecified = virDomainDeviceAddressIsValid(&net->info,
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
.. here?
> + bool ambiguousPartial = false;
> + g_auto(virBuffer) errBuf = VIR_BUFFER_INITIALIZER;
> +
> + /* initialize the matches array to contain the indexes for all
> + * NetDefs. We will then go through this array for each field we
> + * want to match on, removing the indexes of NetDefs that don't
> + * match for that field. At the end we'll have no match, one
> + * match, or ambiguous (multiple) matches.
> + */
> + for (i = 0; i < def->nnets; i++)
> + g_array_append_val(matches, i);
> +
> + for (gi = 0; gi < matches->len;) {
> +
> + i = g_array_index(matches, size_t, gi);
> + /*
> + * mac/pci/ccw addresses must always match if they are
> + * specified in the search template object
> + */
>
> - for (i = 0; i < def->nnets; i++) {
> if (MACAddrSpecified &&
> - virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
> + virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0) {
> + g_array_remove_index(matches, gi);
> continue;
> + }
>
> if (PCIAddrSpecified &&
> !virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
> - &net->info.addr.pci))
> + &net->info.addr.pci)) {
> + g_array_remove_index(matches, gi);
> continue;
> + }
>
> if (CCWAddrSpecified &&
> !virDomainDeviceCCWAddressEqual(&def->nets[i]->info.addr.ccw,
> - &net->info.addr.ccw))
> - continue;
> -
> - if (net->info.alias &&
> - STRNEQ_NULLABLE(def->nets[i]->info.alias, net->info.alias)) {
> - continue;
> + &net->info.addr.ccw)) {
> + g_array_remove_index(matches, gi);
> + continue;
> }
>
> - if (matchidx >= 0) {
> - /* there were multiple matches on mac address, and no
> - * qualifying guest-side PCI/CCW address was given, so we must
> - * fail (NB: a USB address isn't adequate, since it may
> - * specify only vendor and product ID, and there may be
> - * multiples of those.
> - */
> - if (MACAddrSpecified) {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("multiple devices matching MAC address %s found"),
> - virMacAddrFormat(&net->mac, mac));
> - } else {
> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> - _("multiple matching devices found"));
> - }
> + /* this NetDef matches all the mandatory fields, so keep it for now */
> + gi++;
> + }
>
> - return -1;
> - }
> + if (matches->len == 0)
> + goto cleanup;
>
> - matchidx = i;
> - }
> -
> - if (matchidx < 0) {
> - if (MACAddrSpecified && PCIAddrSpecified) {
> - virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device matching MAC address %s found on "
> - VIR_PCI_DEVICE_ADDRESS_FMT),
> - virMacAddrFormat(&net->mac, mac),
> - net->info.addr.pci.domain,
> - net->info.addr.pci.bus,
> - net->info.addr.pci.slot,
> - net->info.addr.pci.function);
> - } else if (MACAddrSpecified && CCWAddrSpecified) {
> - virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device matching MAC address %s found on "
> - VIR_CCW_DEVICE_ADDRESS_FMT),
> - virMacAddrFormat(&net->mac, mac),
> - net->info.addr.ccw.cssid,
> - net->info.addr.ccw.ssid,
> - net->info.addr.ccw.devno);
> - } else if (PCIAddrSpecified) {
> - virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device found on " VIR_PCI_DEVICE_ADDRESS_FMT),
> - net->info.addr.pci.domain,
> - net->info.addr.pci.bus,
> - net->info.addr.pci.slot,
> - net->info.addr.pci.function);
> - } else if (CCWAddrSpecified) {
> - virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device found on " VIR_CCW_DEVICE_ADDRESS_FMT),
> - net->info.addr.ccw.cssid,
> - net->info.addr.ccw.ssid,
> - net->info.addr.ccw.devno);
> - } else if (MACAddrSpecified) {
> - virReportError(VIR_ERR_DEVICE_MISSING,
> - _("no device matching MAC address %s found"),
> - virMacAddrFormat(&net->mac, mac));
> - } else {
> - virReportError(VIR_ERR_DEVICE_MISSING, "%s",
> - _("no matching device found"));
> + /* A "partial match" is when everything but alias matches. When
> + * we've already determined that exactly one NetDef matches
> + * everything else that was specified in the object we're trying
> + * to match, counting this "partial match" as a success allows for
> + * finding an interface based on MAC/pci address, with the intent
> + * to update the alias (i.e. the alias name doesn't match because
> + * it's the *new* alias name we want to change to).
> + *
> + * We can't *always* ignore the alias though, since it is
> + * sometimes used to disambiguate between multiple NetDefs when
> + * searching for a NetDef to remove (rather than update) - in
> + * particular if there are multiple NetDefs with the same MAC
> + * address, and the caller didn't specify PCI address (because
> + * they don't know it). Obviously when it's necessary to look at
> + * the alias in order to disambiguate between two NetDefs, it
> + * won't be possible to update the alias; updating other
> + * attributes, or unplugging the device, shouldn't be a problem
> + * though.
> + */
> + if (matchPartial) {
> + if (matches->len == 1)
> + goto cleanup;
> +
> + /* by the time we've checked the multiple remaining NetDefs
> + * for a match to the alias, we'll no longer have the
> + * information about ambiguity in the match prior to checking
> + * aliases, but we'll need to know that in order to report the
> + * proper error, so save that fact now.
> + */
> + ambiguousPartial = true;
> + }
> +
> + if (net->info.alias) {
> + for (gi = 0; gi < matches->len;) {
> + i = g_array_index(matches, size_t, gi);
> +
> + if (STRNEQ_NULLABLE(def->nets[i]->info.alias, net->info.alias))
> + g_array_remove_index(matches, gi);
> + else
> + gi++;
> }
> }
> - return matchidx;
> +
> + cleanup:
> + if (matches->len == 1)
> + return g_array_index(matches, size_t, 0);
I wonder if this can be changed so that retval is set before jumping
onto this label. To me, 'goto cleanup' means an error state. But maybe
I'm too picky.
> +
> + /* make a string describing all the match terms */
Unfortunatelly, it's not translation friendly.
> + if (MACAddrSpecified)
> + virBufferAsprintf(&errBuf, "MAC address '%s' ", virMacAddrFormat(&net->mac, mac));
> +
> + if (PCIAddrSpecified) {
> + g_autofree char *pciAddrStr = virPCIDeviceAddressAsString(&net->info.addr.pci);
> +
> + virBufferAsprintf(&errBuf, "PCI address '%s' ", pciAddrStr);
> + }
> +
> + if (CCWAddrSpecified) {
> + virBufferAsprintf(&errBuf, "CCW address '" VIR_CCW_DEVICE_ADDRESS_FMT "' ",
> + net->info.addr.ccw.cssid,
> + net->info.addr.ccw.ssid,
> + net->info.addr.ccw.devno);
> + }
> +
> + if (net->info.alias)
> + virBufferAsprintf(&errBuf, "alias name '%s' ", net->info.alias);
> +
> + virBufferTrim(&errBuf, " ");
> +
> + /* we either matched too many... */
> +
> + if (matches->len > 1 || ambiguousPartial) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("found multiple devices matching requested <interface> (%s)"),
> + virBufferContentAndReset(&errBuf));
But the error message per-se is, so I guess it's okay.
> + return -1;
> + }
> +
> +
> + /* ...or not enough
> + * (at this point we know matches->len == 0, no need to check)
> + */
> +
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("found no devices matching requested <interface> (%s)"),
> + virBufferContentAndReset(&errBuf));
> + return -1;
> }
Honestly, this is not the most beautiful patch I've seen, but also I
don't have any idea how to make it look better, so I'll just shut up, and:
Reviewed-by: Michal Privoznik <mprivozn at redhat.com>
Michal
More information about the libvir-list
mailing list