[libvirt] [PATCH] util: only register callbacks for CREATE operations in virnetdevmacvlan.c
From: "D. Herrendoerfer" <d.herrendoerfer@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@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) { + /*Only directly register upon a create - migration and restart are handled elsewhere*/ + if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, + linkdev, vmuuid, virtPortProfile, vmOp) < 0 ) goto disassociate_exit; + } return rc; -- 1.7.7.6
On 04/13/2012 08:41 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer" <d.herrendoerfer@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@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.
From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name> I believe the following is the correct patch for this - when a guest is started from a snapshot or from a saved image, qemuProcessStart is called with VIR_NETDEV_VPORT_PROFILE_OP_RESTORE. From my reading of the code, the callback will have been removed when the guest was previously saved, so it will need to be re-added when it's restored. Dirk - please test this and let me know if it behaves properly. --------------------------------- 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 or RESTORE is requested. Migration and libvirtd restart scenarios are already handled elsewhere. Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 326e29c..879d846 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -945,9 +945,18 @@ create_name: goto disassociate_exit; } - if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, - linkdev, vmuuid, virtPortProfile, vmOp) < 0 ) + if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || + vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE) { + /* Only directly register upon a create or restore (restarting + * a saved image) - migration and libvirtd restart are handled + * elsewhere. + */ + if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, + linkdev, vmuuid, + virtPortProfile, + vmOp) < 0 ) goto disassociate_exit; + } return rc; -- 1.7.10
On Apr 18, 2012, at 10:53 AM, Laine Stump wrote:
From: "D. Herrendoerfer" <d.herrendoerfer@herrendoerfer.name>
I believe the following is the correct patch for this - when a guest is started from a snapshot or from a saved image, qemuProcessStart is called with VIR_NETDEV_VPORT_PROFILE_OP_RESTORE. From my reading of the code, the callback will have been removed when the guest was previously saved, so it will need to be re-added when it's restored.
Yes. I tested for this. In any case of a disassociation the callback is properly removed.
Dirk - please test this and let me know if it behaves properly.
I just posted an update to my initial patch - sorry for the delay, but I wanted to make sure this does not create any other regressions. My patch is basically identical to yours.
---------------------------------
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 or RESTORE is requested. Migration and libvirtd restart scenarios are already handled elsewhere.
Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 326e29c..879d846 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -945,9 +945,18 @@ create_name: goto disassociate_exit; }
- if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, - linkdev, vmuuid, virtPortProfile, vmOp) < 0 ) + if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || + vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE) { + /* Only directly register upon a create or restore (restarting + * a saved image) - migration and libvirtd restart are handled + * elsewhere. + */ + if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, + linkdev, vmuuid, + virtPortProfile, + vmOp) < 0 ) goto disassociate_exit; + }
return rc;
-- 1.7.10
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
DirkH
From: "D. Herrendoerfer" <d.herrendoerfer@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. A VM restore also leads to an association creation and it is also covered here. Migration and libvirtd restart scenarios are already handeled elsewhere. Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 17ea883..1890759 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -945,9 +945,14 @@ create_name: goto disassociate_exit; } - if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, - linkdev, vmuuid, virtPortProfile, vmOp) < 0 ) - goto disassociate_exit; + if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || + vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE ) { + /* Only directly register upon a create/restore - + migration and restart are handled elsewhere */ + if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, + linkdev, vmuuid, virtPortProfile, vmOp) < 0 ) + goto disassociate_exit; + } return rc; -- 1.7.7.6
On 04/18/2012 07:09 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer"<d.herrendoerfer@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. A VM restore also leads to an association creation and it is also covered here. Migration and libvirtd restart scenarios are already handeled elsewhere.
Signed-off-by: D. Herrendoerfer<d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 17ea883..1890759 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -945,9 +945,14 @@ create_name: goto disassociate_exit; }
- if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, - linkdev, vmuuid, virtPortProfile, vmOp)< 0 ) - goto disassociate_exit; + if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || + vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE ) { + /* Only directly register upon a create/restore - + migration and restart are handled elsewhere */ + if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, + linkdev, vmuuid, virtPortProfile, vmOp)< 0 ) + goto disassociate_exit; + }
return rc;
I'd ACK this assuming this has no negative effects on the 802.1Qbh support. CC'ing Roopa. Stefan
On 04/18/2012 07:09 AM, D. Herrendoerfer wrote:
From: "D. Herrendoerfer" <d.herrendoerfer@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. A VM restore also leads to an association creation and it is also covered here. Migration and libvirtd restart scenarios are already handeled elsewhere.
Signed-off-by: D. Herrendoerfer <d.herrendoerfer@herrendoerfer.name> --- src/util/virnetdevmacvlan.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 17ea883..1890759 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -945,9 +945,14 @@ create_name: goto disassociate_exit; }
- if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, - linkdev, vmuuid, virtPortProfile, vmOp) < 0 ) - goto disassociate_exit; + if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || + vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE ) { + /* Only directly register upon a create/restore - + migration and restart are handled elsewhere */ + if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress, + linkdev, vmuuid, virtPortProfile, vmOp) < 0 ) + goto disassociate_exit; + }
return rc;
ACK and pushed (prior to seeing Stefan's message, but as I understand it, 802.1qbh doesn't use these callbacks at all, and this particular patch is only *decreasing* the number of added callbacks, so if anything it should make the situation better :-) Note that it looks like these callbacks are being registered for any direct interface with a non-null port profile, not just for 802.1qbg. If others don't need it (can someone verify that? If so, I'll send the patch), we should add a qualifying if() (possibly at this same place) to only register if the portprofile type is 802.1qbh, i.e. if ((virPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_8021QBG) && (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE)) { ...
participants (3)
-
D. Herrendoerfer -
Laine Stump -
Stefan Berger