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(-)
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@redhat.com>
Signed-off-by: Laine Stump <laine@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);
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);
+ 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);
+
+ /* make a string describing all the match terms */
+ 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));
+ 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;
}
bool
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 87bc7e8625..da32016b01 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3554,7 +3554,7 @@ virDomainDiskRemove(virDomainDefPtr def, size_t i);
virDomainDiskDefPtr
virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
-int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net);
+int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net, bool matchPartial);
virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char *device);
virDomainNetDefPtr virDomainNetFindByName(virDomainDefPtr def, const char *ifname);
bool virDomainHasNet(virDomainDefPtr def, virDomainNetDefPtr net);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 830634b2bd..bfbfdf94e6 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3861,7 +3861,7 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,
libxl_device_nic_init(&nic);
- if ((detachidx = virDomainNetFindIdx(vm->def, net)) < 0)
+ if ((detachidx = virDomainNetFindIdx(vm->def, net, false)) < 0)
goto cleanup;
detach = vm->def->nets[detachidx];
@@ -3990,7 +3990,7 @@ libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
case VIR_DOMAIN_DEVICE_NET:
net = dev->data.net;
- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ if ((idx = virDomainNetFindIdx(vmdef, net, false)) < 0)
return -1;
/* this is guaranteed to succeed */
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 8e0ec82e0b..d96a6f5f2e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3096,7 +3096,7 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
switch (dev->type) {
case VIR_DOMAIN_DEVICE_NET:
net = dev->data.net;
- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ if ((idx = virDomainNetFindIdx(vmdef, net, true)) < 0)
return -1;
oldDev.data.net = vmdef->nets[idx];
@@ -3150,7 +3150,7 @@ lxcDomainDetachDeviceConfig(virDomainDefPtr vmdef,
case VIR_DOMAIN_DEVICE_NET:
net = dev->data.net;
- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ if ((idx = virDomainNetFindIdx(vmdef, net, false)) < 0)
return -1;
/* this is guaranteed to succeed */
@@ -3962,7 +3962,7 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
const virNetDevVPortProfile *vport = NULL;
virErrorPtr save_err = NULL;
- if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
+ if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net, false)) < 0)
goto cleanup;
detach = vm->def->nets[detachidx];
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f3f8caab44..df69782f9c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7159,7 +7159,7 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
break;
case VIR_DOMAIN_DEVICE_NET:
- if ((idx = virDomainNetFindIdx(vm->def, dev->data.net)) >= 0) {
+ if ((idx = virDomainNetFindIdx(vm->def, dev->data.net, true)) >= 0) {
oldDev.data.net = vm->def->nets[idx];
if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
VIR_DOMAIN_DEVICE_ACTION_UPDATE,
@@ -7459,7 +7459,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
case VIR_DOMAIN_DEVICE_NET:
net = dev->data.net;
- if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
+ if ((idx = virDomainNetFindIdx(vmdef, net, false)) < 0)
return -1;
/* this is guaranteed to succeed */
@@ -7686,7 +7686,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
case VIR_DOMAIN_DEVICE_NET:
net = dev->data.net;
- if ((pos = virDomainNetFindIdx(vmdef, net)) < 0)
+ if ((pos = virDomainNetFindIdx(vmdef, net, true)) < 0)
return -1;
oldDev.data.net = vmdef->nets[pos];
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 58d2abb862..f0cf75e547 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3512,7 +3512,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
g_autoptr(virConnect) conn = NULL;
virErrorPtr save_err = NULL;
- if ((changeidx = virDomainNetFindIdx(vm->def, newdev)) < 0)
+ if ((changeidx = virDomainNetFindIdx(vm->def, newdev, true)) < 0)
goto cleanup;
devslot = &vm->def->nets[changeidx];
olddev = *devslot;
@@ -5664,7 +5664,7 @@ qemuDomainDetachPrepNet(virDomainObjPtr vm,
{
int detachidx;
- if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0)
+ if ((detachidx = virDomainNetFindIdx(vm->def, match, false)) < 0)
return -1;
*detach = vm->def->nets[detachidx];
--
2.30.2
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@redhat.com> > Signed-off-by: Laine Stump <laine@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(-) 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 :|
On 3/23/21 5:37 AM, Daniel P. Berrangé wrote:
> 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.
Okay, I'm fine with that; prefer it, even (truthfully as soon as you
said that, I wondered why I didn't push back on the regression claim - I
guess there's just so much "accidental behavior" that we've ended up
codifying (a couple that come to mind are the tangentially related "user
specified alias names are ignored *unless they being with "ua-", and
"tap device names beginning with "vnet" are ignored), that I've gotten
used to quiet compliance unless it's something I have a strong opinion
about...).
Now I just need to figure out how to sort out the BZ status.
Anyway, the other thing to come out of this is trying out GArray - my
verdict is that 1) I like that the length is part of the GArray rather
than our practice of using a separate variable, and 2) being required to
use a big long macro where you must even specify the type of the element
in order to simply access the element at a specific index is ugly and
cumbersome. I mean, really:
matches[i]
vs
g_array_index(matches, size_t, i)
??
Couldn't they at least have written the macro to use typeof(*matches)?
>
>>
>> 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@redhat.com>
>> Signed-off-by: Laine Stump <laine@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(-)
>
> Regards,
> Daniel
>
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@redhat.com>
> Signed-off-by: Laine Stump <laine@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@redhat.com>
Michal
On 3/23/21 8:44 AM, Michal Privoznik wrote: > > 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, I didn't like writing it either. Anyway, I like Dan's idea better - "that's not a regression, it's a bonus bugfix for a bug that someone mistakenly thought was a feature".
© 2016 - 2024 Red Hat, Inc.