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

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



On Wed, Jan 25, 2012 at 04:09:22PM +0100, Hendrik Schwartke wrote:
> I wrote a patch to change the mapping between a virtual bridge
> interface and the host bridge while the host is up. It's based on
> commit 6fba577e505611e6c25c68e322942eab7754de7e. The host and the
> interface definition I used for testing are also attached.
> I would be glad if the patch could be added to the repo.

Great !

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4b60839..f791795 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -40,6 +40,7 @@
>  #include "qemu_cgroup.h"
>  #include "locking/domain_lock.h"
>  #include "network/bridge_driver.h"
> +#include "util/virnetdevbridge.h"
> 
>  #define VIR_FROM_THIS VIR_FROM_QEMU
> 
> @@ -1163,6 +1164,28 @@ static virDomainNetDefPtr
> qemuDomainFindNet(virDomainObjPtr vm,
>      return NULL;
>  }
> 
> +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(virNetDevBridgeRemovePort(oldbridge, olddev->ifname)<0) {
> +        qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                        _("unable to remove port from bridge"));

Good idea to include the bridge device name too

> +        return -1;
> +    }
> +    if(virNetDevBridgeAddPort(newbridge, newdev->ifname)<0) {
> +        qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                        _("unable to add port to bridge"));
> +        return -1;
> +    }

Hmm, we'd end up with no NIC backend command at all here.
So we'd probably want to move the VIR_FREE() line before
here, so if we fail half-way through, we still update
the XML. To avoid stupid mistakes we also probably want
to check for existance of the target bridge before unplugging
fromthe source

> +    VIR_FREE(olddev->data.bridge.brname);
> +    olddev->data.bridge.brname=strdup(newbridge);

We could just steal newbridge from the newdev and
avoid the OOM potential

> +    return 0;
> +}

Various whitespace fixes

 - For assignment, we prefer 'foo = bar' instead of 'foo=bar'
 - For conditionals  'if (' instead of 'if('
 - For tests  "foo < 0" instead of "foo<0"


So all in all I think I'd write this as 

  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)) {
        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;
    }
    VIR_FREE(olddev->data.bridge.brname);
    if (virNetDevBridgeAddPort(newbridge, newdev->ifname) < 0) {
        qemuReportError(VIR_ERR_OPERATION_FAILED,
                        _("unable to add port to bridge"), newbridge);
        return -1;
    }
    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 +1316,12 @@ int qemuDomainChangeNet(struct qemud_driver *driver,
>          return -1;
>      }
> 
> +    if(olddev->type==VIR_DOMAIN_NET_TYPE_BRIDGE
> + && dev->type==VIR_DOMAIN_NET_TYPE_BRIDGE
> + && STRNEQ(olddev->data.bridge.brname, dev->data.bridge.brname)) {
> +        qemuDomainChangeNetBridge(olddev, dev);
> +    }

And use STRNEQ_NULLABLE here to avoid risk of NULL brname field,
and if qemuDomainChangeNetBridge() returns -1, we need to return
to the caller here.

> +
>      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);
> 
> 
> Host definition:
> <domain type='kvm'>
> <name>test</name>
> <memory>32768</memory>
> <currentMemory>32768</currentMemory>
> <vcpu>1</vcpu>
> <os>
> <type arch='x86_64' machine='pc-0.12'>hvm</type>
> <boot dev='cdrom'/>
> <bootmenu enable='no'/>
> </os>
> <features>
> <acpi/>
> <apic/>
> <pae/>
> </features>
> <clock offset='utc'/>
> <on_poweroff>destroy</on_poweroff>
> <on_reboot>restart</on_reboot>
> <on_crash>restart</on_crash>
> <devices>
> <emulator>/usr/bin/kvm</emulator>
> <controller type='ide' index='0'>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> </controller>
> <interface type='bridge'>
> <mac address='52:54:00:ab:cd:02'/>
> <source bridge='br0'/>
> <target dev='testif'/>
> <link state='up'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> </interface>
> <serial type='pty'>
> <target port='0'/>
> </serial>
> <console type='pty'>
> <target type='serial' port='0'/>
> </console>
> <input type='tablet' bus='usb'/>
> <input type='mouse' bus='ps2'/>
> <graphics type='vnc' port='-1' autoport='yes'/>
> <video>
> <model type='cirrus' vram='9216' heads='1'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> </video>
> <memballoon model='virtio'>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
> </memballoon>
> </devices>
> </domain>
> 
> 
> Interface definition:
> <interface type="bridge">
> <mac address="52:54:00:ab:cd:02"/>
> <source bridge="br1"/>
> <target dev="testif"/>
> <link state="up"/>
> <alias name="net0"/>
> <address type="pci" domain="0x0000" bus="0x00" slot="0x03" function="0x0"/>
> </interface>
> 
> 
> 
> Best regards
> Hendrik Schwartke
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
|: 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]