On 9/18/24 17:26, Laine Stump wrote: > Attempts to use update-device to modify just the link state of a guest > interface were failing due to a supposed attempt to modify something > in the interface that can't be modified live (even though the only > thing that was changing was the link state, which *can* be modified > live). > > It turned out that this failure happened because the guest interface > in question was type='network', and the network in question was a > 'direct' network that provides each guest interface with one device > from a pool of network devices. As a part of qemuDomainChangeNet() we > would always allocate a new port from the network driver for the > updated interface definition (by way of calling > virDomainNetAllocateActualDevice(newdev)), and this new port (ie the > ActualNetDef in newdev) would of course be allocated a new host device > from the pool (which would of course be different from the one > currently in use by the guest interface (in olddev)). Because direct > interfaces don't support changing the host device in a live update, > this would cause the update to fail. > > The solution to this is to realize that as long as the interface > doesn't get switched to a different network as a part of the update, > the network port information (ie the ActualNetDef) will not change as > a part of updating the guest interface itself. So for sake of > comparison we can just point the newdev at the ActualNetDef of olddev, > and then clear out one or the other when we're done (to avoid a double > free or, more likely, attempt to reference freed memory). > > (If, on the other hand, the name of the network has changed, or if the > interface type has changed to type='network' from something else, then > we *do* need to allocate a new port (actual device) from the network > driver (as we used to do in all cases when the new type was > 'network'), and also indicate that we'll need to replace olddev in the > domain with newdev (because either of these changes is major enough > that we shouldn't just try to fix up olddev) > > Resolves: https://issues.redhat.com/browse/RHEL-7036 > Signed-off-by: Laine Stump <laine@xxxxxxxxxx> > --- > src/qemu/qemu_hotplug.c | 92 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 80 insertions(+), 12 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 75b97cf736..a187466c5b 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -3675,6 +3675,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, > virDomainNetDef **devslot = NULL; > virDomainNetDef *olddev; > virDomainNetType oldType, newType; > + bool actualSame = false; > bool needReconnect = false; > bool needBridgeChange = false; > bool needFilterChange = false; > @@ -3895,15 +3896,49 @@ qemuDomainChangeNet(virQEMUDriver *driver, > * free it if we fail for any reason > */ > if (newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > - if (!(conn = virGetConnectNetwork())) > - goto cleanup; > - if (virDomainNetAllocateActualDevice(conn, vm->def, newdev) < 0) > - goto cleanup; > - } > + if (olddev->type == VIR_DOMAIN_NET_TYPE_NETWORK > + && STREQ(olddev->data.network.name, newdev->data.network.name)) { > + /* old and new are type='network', and the network name > + * hasn't changed. In this case we *don't* want to get a > + * new port ("actual device") from the network because we > + * can use the old one (since it hasn't changed). > + * > + * So instead we just duplicate *the pointer to* the > + * actualNetDef from olddev to newdev so that comparisons > + * of actualNetDef will show no change. If the update is > + * successful, we will clear the actualNetDef pointer from > + * olddev before destroying it (or if the update fails, > + * then we need to clear the pointer from newdev before > + * destroying it) > + */ > + newdev->data.network.actual = olddev->data.network.actual; > + memcpy(newdev->data.network.portid, olddev->data.network.portid, > + sizeof(newdev->data.network.portid)); I thought we had a function that copies over .actual, but apparently I remembered it wrong. > + actualSame = true; /* old and new actual are pointing to same object */ > + } else { > + /* either the olddev wasn't type='network', or else the > + * name of the network changed. In this case we *do* want > + * to get a new port from the new network (because we know > + * that it *will* change), and then if the update is > + * successful, we will release the port ("actual device") > + * in olddev. Or if the update is a failure, we will > + * release this new port > + */ > + if (!(conn = virGetConnectNetwork()) > + || virDomainNetAllocateActualDevice(conn, vm->def, newdev) < 0) { nitpick, The or operator should go onto the previous line. > + goto cleanup; > + } > Michal
