[libvirt] [PATCH] Using Libvirt to change the bridge a virtual network card of a running vm is connected to

[Laine Stump]

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:
> Of course:
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4b60839..edf7433 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 "util/virnetdev.h"
> +#include "util/virnetdevbridge.h"

"util/" is unnecessary, since the util directory is already in the
include path.

>
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>
> @@ -1163,6 +1165,44 @@ static virDomainNetDefPtr
> qemuDomainFindNet(virDomainObjPtr vm,
>      return NULL;
>  }
>
> +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev,
> +                              virDomainNetDefPtr newdev)

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).

> +{
> +    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)) {
> +        qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                        _("unable to remove port from bridge"));
> +        return -1;
> +    }
> +
> +    if (oldbridge&&
> +        virNetDevBridgeRemovePort(oldbridge, olddev->ifname)<  0) {
> +        qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                        _("unable to remove port from bridge %s"),
> oldbridge);
> +        return -1;
> +    }
> +    if (virNetDevBridgeAddPort(newbridge, newdev->ifname)<  0) {
> +        qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                        _("unable to add port to bridge %s"),
> newbridge);
> +        if (virNetDevBridgeAddPort(oldbridge, olddev->ifname)<  0) {
> +            qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                            _("unable to recover former state by
> adding port to bridge %s"), oldbridge);
> +            VIR_FREE(olddev->data.bridge.brname);

This free is unnecessary - the parent object (and all its minions) will
be freed by the caller anyway.

>
> +        }
> +        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,
> @@ -1293,6 +1333,13 @@ 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;
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 0310361..1e1f75c 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -80,6 +80,8 @@ int qemuDomainChangeNetLinkState(struct qemud_driver
> *driver,
>                                   virDomainObjPtr vm,
>                                   virDomainNetDefPtr dev,
>                                   int linkstate);
> +int qemuDomainChangeNetBridge(virDomainNetDefPtr olddev,
> +                              virDomainNetDefPtr newdev);
>  int qemuDomainDetachPciDiskDevice(struct qemud_driver *driver,
>                                    virDomainObjPtr vm,
>                                    virDomainDeviceDefPtr dev);
>
>
>
>
> On 26.01.2012 15:18, Eric Blake wrote:
>> On 01/26/2012 07:10 AM, Hendrik Schwartke wrote:
>>> I fixed the patch and added it to
>>> https://bugzilla.redhat.com/show_bug.cgi?id=784767.
>> Thanks.  It's easier to review if we don't have to click through the
>> link, so can you also send the updated patch to this list?
>>
>
> -- 
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>