Sorry for taking so long to actually review this patch. (The fact that it didn't apply cleanly (neither the version you sent in email, nor the version attached to https://bugzilla.redhat.com/784767) put it just beyond the threshold of action (the method you used to generate/send the patch resulted in a corrupt patch that couldn't be applied with "git am" or "git am -3"). In general, it's best to send patches to the mailing list with "git send-email", or failing that, by running "git format-patch", then attaching the result to a mail message (git send-email is *greatly* preferred).) (In the end, I pasted the chunks into my work tree by hand.) Your patch addresses one specific case (interface type is 'bridge', and doesn't change, but the bridge name does change) of a large class that could/should be handled (e.g. switch between type='network' and type='bridge', optionally bounce the link to force a DHCP refresh, optionally detach/reattach the device if required (e.g. to switch between macvtap and standard bridge connections)). My opinion is that the existing patch can go in as a useful starting point, and we can expand on it (if someone else has a differing opinion, please say so!) Other than the minimal functionality, I only saw few nits in the code, which can be fixed up before pushing, so ACK from me, but we may want to wait until after the freeze is over and 0.9.11 is released to push. On 01/26/2012 10:29 AM, Hendrik Schwartke wrote: "util/" is unnecessary, since the util directory is already in the include path. Although the existing qemuDomainChangeLinkState is a global function, it is unnecessarily so, as is this new function. I think it should just be static (and qemuDomainChangeLinkState should also be made static). This free is unnecessary - the parent object (and all its minions) will be freed by the caller anyway.

From: Hendrik Schwartke <hendrik(a)os-t.de&gt; Previously the only attribute of a network device that could be modified by virUpdateDeviceFlags() ("virsh update-device") was the link state; attempts to change any other attribute would log an error and fail. (One notable exception to this was changing the bridge used by an interface of type='bridge' - that wasn't flagged as an error, but still did nothing.) This patch adds recognition of a change in bridge device name, and supports reconnecting the guest's interface to the new device. --- This had too many modifications from the original patch for me to push without sending in for a 3rd party review. Here are the changes from Hendrik's original patch: Things I noted in my original review: 1) remove unnecessary "util/" from #includes 2) change qemuDomainChangeNetBridge from global to static, since it's only used in the one file. 3) Don't free olddev->data.bridge.brname after failure to add the tap to the new bridge, since the caller will free all of olddev anyway. Things I didn't notice until after I reviewed and ACKed (but fortunately before I pushed): 4) When adding the tap to the new bridge, use olddev->ifname instead of newdev->ifname - the one in newdef is evilly cleared out by the parser, and ifname isn't allowed to change anyway, so using olddev->ifname is proper. 5) Add a case for VIR_DOMAIN_NET_TYPE_BRIDGE to the switch that checks for changes to disallowed items for each type of interface. Even before adding the new functionality, absence of this case had meant that attempts to change link state of a type='bridge' interface would have failed (so I was right that this patch fixes an existing bug, but was wrong about exactly what the bug was.) I have run make check and done functional checking on this code - it does properly change the bridge of an existing guest interface without needing to detach it. src/qemu/qemu_hotplug.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 38163ba..66837c4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -40,6 +40,8 @@ #include "qemu_cgroup.h" #include "locking/domain_lock.h" #include "network/bridge_driver.h" +#include "virnetdev.h" +#include "virnetdevbridge.h" #include "virnetdevtap.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1191,6 +1193,40 @@ static virDomainNetDefPtr qemuDomainFindNet(virDomainObjPtr vm, return NULL; } +static +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev) +{ + const char *oldbridge = olddev->data.bridge.brname; + const char *newbridge = newdev->data.bridge.brname; + + VIR_DEBUG("Change bridge for interface %s: %s -> %s", + olddev->ifname, oldbridge, newbridge); + + if (virNetDevExists(newbridge) != 1) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("bridge %s doesn't exist"), newbridge); + return -1; + } + + if (oldbridge && + virNetDevBridgeRemovePort(oldbridge, olddev->ifname) < 0) { + return -1; + } + if (virNetDevBridgeAddPort(newbridge, olddev->ifname) < 0) { + if (virNetDevBridgeAddPort(oldbridge, olddev->ifname) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("unable to recover former state by adding port" + "to bridge %s"), oldbridge); + } + return -1; + } + VIR_FREE(olddev->data.bridge.brname); + olddev->data.bridge.brname = newdev->data.bridge.brname; + newdev->data.bridge.brname = NULL; + return 0; +} + int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, @@ -1279,6 +1315,16 @@ int qemuDomainChangeNet(struct qemud_driver *driver, break; + case VIR_DOMAIN_NET_TYPE_BRIDGE: + /* allow changing brname, but not portprofile */ + if (!virNetDevVPortProfileEqual(olddev->data.bridge.virtPortProfile, + dev->data.bridge.virtPortProfile)) { + qemuReportError(VIR_ERR_NO_SUPPORT, + _("cannot modify bridge network device configuration")); + return -1; + } + break; + case VIR_DOMAIN_NET_TYPE_INTERNAL: if (STRNEQ_NULLABLE(olddev->data.internal.name, dev->data.internal.name)) { qemuReportError(VIR_ERR_NO_SUPPORT, @@ -1321,6 +1367,14 @@ int qemuDomainChangeNet(struct qemud_driver *driver, return -1; } + if (olddev->type == VIR_DOMAIN_NET_TYPE_BRIDGE + && dev->type == VIR_DOMAIN_NET_TYPE_BRIDGE + && STRNEQ_NULLABLE(olddev->data.bridge.brname, + dev->data.bridge.brname)) { + if ((ret = qemuDomainChangeNetBridge(olddev, dev)) < 0) + return ret; + } + if (olddev->linkstate != dev->linkstate) { if ((ret = qemuDomainChangeNetLinkState(driver, vm, olddev, dev->linkstate)) < 0) return ret; -- 1.7.7.6

On 03/30/2012 10:39 AM, Daniel P. Berrange wrote: Okay, I've redone the auditing calls, placing them immediately after each call to add/remove port, calling regardless of whether the operation was successful or not. I haven't addressed the higher level problem of adding bridge info to the audit message (although I have made sure that the NetDef object sent to the audit function has the proper information - that's why the brname is moved around at different times than previously). Does this (squashed into the initial patch that had no auditing) look adequate? (I've tested this code, including failure to attach to the new bridge (by trying to attach to "eth0"), and it does work as you suggest).