[libvirt] [PATCH] util: only register callbacks for CREATE operations in virnetdevmacvlan.c

[Laine Stump]

On 04/13/2012 08:41 AM, D. Herrendoerfer wrote:
> From: "D. Herrendoerfer" <d.herrendoerfer at herrendoerfer.name>
>
> currently upon a migration a callback is created when a 802.1qbg link
> is set to PREASSOCIATE, this should not happen because this is
> a no-op on most switches, and does not lead to an ASSOCIATE state.
> This patch only creates callbacks when CREATE is requested.
> Migration and libvirtd restart scenarios are already handeled elsewhere.
>
> Signed-off-by: D. Herrendoerfer <d.herrendoerfer at herrendoerfer.name>
> ---
>  src/util/virnetdevmacvlan.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 17ea883..73f41ff 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -945,9 +945,12 @@ create_name:
>          goto disassociate_exit;
>      }
>  
> -    if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress,
> -                                         linkdev, vmuuid, virtPortProfile, vmOp) < 0 )
> +    if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE) {


I'm not sure this is going to work for all startup scenarios. For
example, What about when a guest is restarted
(qemuDomainSaveImageStartVM)? The call to qemuProcessStart sets the vmOp
to VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, but I would assume that when a
guest is saved (implying that its memory image was written to disk and
the qemu process was terminated), the callback was cancelled (if not,
that's a bug. Note also that it's possible the host may be rebooted
while the guest isn't running, and the guest then resumed from its image
on disk).


> +        /*Only directly register upon a create - migration and restart are handled elsewhere*/
> +        if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress,
> +                                                         linkdev, vmuuid, virtPortProfile, vmOp) < 0 )


A more minor point - I know that, due to the length of the function
name, it's impossible to make this all fit inside 80 columns while also
satisfying libvirt's indenting standards, but it would be nice to wrap
the comment at 80 columns (and put a space after the opening /* and
before the closing */).

>          goto disassociate_exit;
> +    }
>  
>      return rc;
>  

I don't have the hardware to actually test this, but I would like to see
either a change that's been tested to show proper operation when a guest
goes through the "virsh save xxx xxx.save; virsh restore xxx.save" (or
"virsh managedsave xxx; virsh start xxx") cycle, or your acknowledgement
that such a sequence operates properly without any change.