On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
> From: Hendrik Schwartke <hendrik@xxxxxxx>
>
> 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;
> + }
I think you need to emit 2 audit notifications here, one for the bridge
being removed and one for the bridge being added.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list