[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2] qemu: support live change of the bridge used by a guest network device



On Thu, Mar 29, 2012 at 04:38:50PM -0400, Laine Stump wrote:
> From: Hendrik Schwartke <hendrik os-t de>
> 
> 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]