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

Re: [libvirt] [PATCHv2 3/4] qemu: reorganize qemuDomainChangeNet



On 10/14/2012 11:23 AM, Laine Stump wrote:
> (V1 of this patch is in the following thread:
> 
>   https://www.redhat.com/archives/libvir-list/2012-October/msg00542.html
> 
> I'm posting it because PATCH 4/4 of that series attempts to use
> apparently non-existent/non-working functionality in qemu; modifying
> PATCH 3/4 slightly gives back some of the functionality lost by not
> having 4/4. Once this is ACKed, I will push 1-3 only, and leave 4/4
> untilits problem is discovered/resolved)
> 
> ****
> 
> This patch resolves:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=805071
> 
> to the extent that it can be resolved with current qemu functionality.
> It attempts to detect as many situations as possible when the simple
> operation of disconnecting an existing tap device from one bridge and
> attaching it to another will satisfy the change requested in
> virDomainUpdateDeviceFlags() for a network device. Before this patch,
> that situation could only be detected if the pre-change interface
> *and* the post-change interface definition were both "type='bridge'".
> After this patch, it can also be detected if the before or after
> interfaces are any combination of type='bridge' and type='network'
> (the networks can be <forward mode='nat|route|bridge'>, as long as
> they use a Linux host bridge and not macvtap connections).
> 
> This extra effort is especially useful since the recent discovery that
> a netdev_del+netdev_add combo (to reconnect the network device with
> completely different hostside configuration) doesn't work properly
> with current qemu (1.2) unless it is accompanied by the matching
> device_del+device_add - see this mailing list message for details:
> 
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-10/msg02355.html
> 
> (A slight modification of the patch referenced there has been prepared
> to apply on top of this patch, but won't be pushed until qemu can be
> made to work with it.)
> 
> * qemuDomainChangeNet needs access to the virDomainDeviceDef that
> holds the new netdef (so that it can clear out the virDomainDeviceDef
> if it ends up using the NetDef to replace the original), so the
> virDomainNetDefPtr arg is replaced with a virDomainDeviceDefPtr.
> 
> * qemuDomainChangeNet previously checked for *some* changes to the
> interface config, but this check was by no means complete. It was also
> a bit disorganized.
> 
> This refactoring of the code is (I believe) complete in its check of
> all NetDef attributes that might be changed, and either returns a
> failure (for changes that are simply impossible), or sets one of three
> flags:
> 
>   needLinkStateChange - if the device link state needs to go up/down
>   needBridgeChange    - if everything else is the same, but it needs
>                         to be connected to a difference linux host
>                         bridge
>   needReconnect       - if the entire host side of the device needs
>                         to be torn down and reconstructed (currently
>                         non-working, as mentioned above)
> 
> Note that this function will refuse to make any change that requires
> the *guest* side of the device to be detached (e.g. changing the PCI
> address or mac address). Those would be disruptive enough to the guest
> that it's reasonable to require an explicit detach/attach sequence
> from the management application.
> 
> * As mentioned above, qemuDomainChangeNet also does its best to
> understand when a simple change in attached bridge for the existing
> tap device will work vs. the need to completely tear down/reconstruct
> the host side of the device (including tap device).
> 
> This patch *does not* implement the "reconnect" code anyway - there is
> a placeholder that turns that into an error. Rather, the purpose of
> this patch is to replicate existing behavior with code that is ready
> to have that functionality plugged in in a later patch.
> 
> * The expanded uses for qemuDomainChangeNetBridge meant that it needed

s/it/it's/

> to be enhanced as well - it no longer replaces the original brname
> string in olddev with the new brname; instead, it relies on the
> caller to replace the *entire* olddev with newdev (since we've gone
> to great lengths to assure they are functionally identical other
> than the name of the bridge, this is now not only safe, but more
> correct). Additionally, qemuDomainNetChangeBridge can now set the
> bridge for type='network' interfaces as well as plain type='bridge'
> interfaces. (Note that I had to make this change simultaneous to the
> reorganization of qemuDomainChangeNet because the two are too
> closely intertwined to separate).
> ---
>  src/qemu/qemu_driver.c  |   2 +-
>  src/qemu/qemu_hotplug.c | 502 ++++++++++++++++++++++++++++++++++++++----------
>  src/qemu/qemu_hotplug.h |   4 +-
>  3 files changed, 401 insertions(+), 107 deletions(-)
> 

[...]

> +    if (olddev->type == newdev->type && oldType == newType) {
>  
> +        /* if type hasn't changed, check the relevant fields for the type */
> +        switch (newdev->type) {
> +        case VIR_DOMAIN_NET_TYPE_USER:
> +            break;
> +
> +        case VIR_DOMAIN_NET_TYPE_ETHERNET:
> +            if (STRNEQ_NULLABLE(olddev->data.ethernet.dev,
> +                                newdev->data.ethernet.dev) ||
> +                STRNEQ_NULLABLE(olddev->script, newdev->script) ||

No need to check this, it was checked few lines before, but no harm done
here.

Everything looks good, so ACK.

Martin


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