External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-10-24 06:53:11 +00:00
Code Issues Packages Projects Releases Wiki Activity

qemu: rework needBridgeChange/needReconnect decisions in qemuDomainChangeNet()

Browse Source 
This patch simplifies (?) the of qemuDomainChangeNet() code while
fixing some incorrect decisions about exactly when it's necessary to
re-attach an interface's bridge device, or to fail the device update
(needReconnect[*]) because the type of connection has changed (or
within bridge and direct (macvtap) type because some attribute of the
connection has changed that can't actually be modified after the
tap/macvtap device of the interface is created).

Example 1: it's pointless to require the bridge device to be
reattached just because the interface has been switched to a different
network (i.e. the name of the network is different), since the new
network could be using the same bridge as the old network (very
uncommon, but technically possible). Instead we should only care if
the name of the *bridge device* changes (or if something in
<virtualport> changes - see Example 3).

Example 2: wrt changing the "type" of the interface, a change should
be allowed if old and new type both used a bridge device (whether or
not the name of the bridge changes), or if old and new type are both
"direct" *and* the device being linked and macvtap mode remain the
same. Any other change in interface type cannot be accommodated and
should be a failure (i.e. needReconnect).

Example 3: there is no valid reason to fail just because the interface
has a <virtualport> element - the <virtualport> could just say
"type='openvswitch'" in both the before and after cases (in which case
it isn't a change by itself, and so is completely acceptable), and
even if the interfaceid changes, or the <virtualport> disappears
completely, that can still be reconciled by simply re-attaching the
bridge device. (If, on the other hand, the modified <virtualport> is
for a type='direct' interface, we can't domodify that, and so must
fail (needReconnect).)

(I tried splitting this into multiple patches, but they were so
intertwined that the intermediate patches made no sense.)

[*] "needReconnect" was a flag added to this function way back in
2012, when I still believed that QEMU might someday support connecting
a new & different device backend (the way the virtual device connects
to the host) to an already existing guest netdev (the virtual device
as it appears to the guest). Sadly that has never happened, so for the
purposes of qemuDOmainChangeNet() "needReconnect" is equivalent to
"fail".

Resolves: https://issues.redhat.com/browse/RHEL-7036
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
Laine Stump 2024-09-12 20:58:30 -04:00
parent 601f4160b9
commit c7ea694f7d
1 changed files with 72 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

120
src/qemu/qemu_hotplug.c
View File

@ -3695,6 +3695,8 @@ qemuDomainChangeNet(virQEMUDriver *driver,
virDomainNetDef **devslot = NULL;
virDomainNetDef *olddev;
virDomainNetType oldType, newType;
const char *oldBridgeName = NULL;
const char *newBridgeName = NULL;
bool actualSame = false;
bool needReconnect = false;
bool needBridgeChange = false;
@ -3962,6 +3964,9 @@ qemuDomainChangeNet(virQEMUDriver *driver,
newType = virDomainNetGetActualType(newdev);
oldBridgeName = virDomainNetGetActualBridgeName(olddev);
newBridgeName = virDomainNetGetActualBridgeName(newdev);
if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
newType == VIR_DOMAIN_NET_TYPE_VDPA) {
/* can't turn it into a type='hostdev' or type='vdpa' interface */
@ -3993,13 +3998,6 @@ qemuDomainChangeNet(virQEMUDriver *driver,
break;
case VIR_DOMAIN_NET_TYPE_NETWORK:
if (STRNEQ(olddev->data.network.name, newdev->data.network.name)) {
if (virDomainNetGetActualVirtPortProfile(newdev))
needReconnect = true;
else
needBridgeChange = true;
}
if (STRNEQ_NULLABLE(olddev->data.network.portgroup,
newdev->data.network.portgroup)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@ -4040,59 +4038,85 @@ qemuDomainChangeNet(virQEMUDriver *driver,
goto cleanup;
}
} else {
/* interface type has changed. There are a few special cases
* where this can only require a minor (or even no) change,
* but in most cases we need to do a full reconnection.
/* The interface type has changed. The only times when this
* wouldn't *always* require completely recreating the backend
* of the netdev (aka needReconnect, which QEMU doesn't
* support anyway) are:
*
* As long as both the new and old types use a tap device
* connected to a host bridge (ie VIR_DOMAIN_NET_TYPE_NETWORK
* or VIR_DOMAIN_NET_TYPE_BRIDGE), we just need to connect to
* the new bridge.
* 1) if oldType and newType are both either _NETWORK or
* _BRIDGE (because both of those end up connecting the tap
* device to a bridge, and that is something that *can* be
* redone without recreating the backend (and will be
* handled below where needBridgeChange is set).
*
* (NB: if either of these is _NETWORK or _BRIDGE, the
* corresponding oldBridgeName/newBridgeName will be
* non-null - this is simpler to check for than checking
* each for both _NETWORK and _BRIDGE)
*
* 2) if oldType and newType are both _DIRECT (and presumably
* will end up specifying the same link device, which is
* checked further down where ActualDirectDev is checked)
*
* These two cases we'll allow through (for further checks
* below)...
*/
if ((oldType == VIR_DOMAIN_NET_TYPE_NETWORK ||
oldType == VIR_DOMAIN_NET_TYPE_BRIDGE) &&
(newType == VIR_DOMAIN_NET_TYPE_NETWORK ||
newType == VIR_DOMAIN_NET_TYPE_BRIDGE)) {
if (!((oldBridgeName && newBridgeName)
|| (oldType == VIR_DOMAIN_NET_TYPE_DIRECT &&
newType == VIR_DOMAIN_NET_TYPE_DIRECT))) {
/* ...for all other combinations, we need a full reconnect
* (which currently isn't, and perhaps probably never will
* be, supported by QEMU, so needReconnect is effectively
* "NOT SUPPORTED")
*/
needReconnect = true;
}
/* whatever else is done, when the interface type has changed,
* we need to replace olddev with newdev
*/
needReplaceDevDef = true;
}
/* tests that need to be done whether or not type or actualType changes */
/* if both new and old use a bridge device */
if (newBridgeName) {
if (STRNEQ_NULLABLE(oldBridgeName, newBridgeName))
needBridgeChange = true;
} else if (oldType == VIR_DOMAIN_NET_TYPE_DIRECT &&
newType == VIR_DOMAIN_NET_TYPE_DIRECT) {
/* this is the case of switching from type='direct' to
* type='network' for a network that itself uses direct
* (macvtap) devices. If the physical device and mode are
* the same, this doesn't require any actual setup
* change. If the physical device or mode *does* change,
* that will be caught in the common section below */
} else {
/* for all other combinations, we'll need a full reconnect */
needReconnect = true;
/* A change in virtportprofile also indicates we probably need
* to re-attach the bridge, e.g. if the profileid or type
* changed.
*/
if (!virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(olddev),
virDomainNetGetActualVirtPortProfile(newdev))) {
needBridgeChange = true;
}
}
/* now several things that are in multiple (but not all)
* different types, and can be safely compared even for those
* cases where they don't apply to a particular type.
/* if the newType is DIRECT then we've already set needReconnect
* if oldType was anything other than DIRECT. We also need to set
* it if the direct mode or anything in the virtportprofile has
* changed.
*/
if (STRNEQ_NULLABLE(virDomainNetGetActualBridgeName(olddev),
virDomainNetGetActualBridgeName(newdev))) {
if (virDomainNetGetActualVirtPortProfile(newdev))
if (newType == VIR_DOMAIN_NET_TYPE_DIRECT) {
if (STRNEQ_NULLABLE(virDomainNetGetActualDirectDev(olddev),
virDomainNetGetActualDirectDev(newdev)) ||
virDomainNetGetActualDirectMode(olddev) != virDomainNetGetActualDirectMode(newdev) ||
!virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(olddev),
virDomainNetGetActualVirtPortProfile(newdev))) {
/* you really can't change much about a macvtap device once it's been created */
needReconnect = true;
else
needBridgeChange = true;
}
}
if (STRNEQ_NULLABLE(virDomainNetGetActualDirectDev(olddev),
virDomainNetGetActualDirectDev(newdev)) ||
virDomainNetGetActualDirectMode(olddev) != virDomainNetGetActualDirectMode(newdev) ||
!virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(olddev),
virDomainNetGetActualVirtPortProfile(newdev))) {
needReconnect = true;
}
/* now several things that are in multiple (but not all) different
* types, and can be safely compared even for those cases where
* they don't apply to a particular type.
*/
if (!virNetDevVlanEqual(virDomainNetGetActualVlan(olddev),
virDomainNetGetActualVlan(newdev))) {