[libvirt] [PATCH 0/3] Couple of virNetDevMacVLanCreateWithVPortProfile fixes
Le sigh. How come this slipped uncaught for that long? Michal Privoznik (3): virNetDevMacVLanCreateWithVPortProfile: Don't mask virNetDevMacVLanTapOpen error virNetDevMacVLanCreateWithVPortProfile: Drop @rc virNetDevMacVLanCreateWithVPortProfile: Drop @ret src/util/virnetdevmacvlan.c | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) -- 2.8.4
https://bugzilla.redhat.com/show_bug.cgi?id=1240439 In this function we create a macvtap device and open its tap device. Possibly multiple times. Not the thing is, if opening the tap device fails, that is virNetDevMacVLanTapOpen() returns a negative value, we unroll all the changes BUT return 0 fooling caller into thinking everything went okay. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c05c67f..bd339c0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -1120,8 +1120,10 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, } if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { - if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) + if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) { + VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ goto disassociate_exit; + } if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr) < 0) { VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ -- 2.8.4
On Tue, Aug 09, 2016 at 07:32:43PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1240439
In this function we create a macvtap device and open its tap device. Possibly multiple times.
Possibly multiple devices? Multiple times sounds like it's the same device.
Not the thing is, if opening the
s/Not/Now/ or s/Not the thing is, i/I/
tap device fails, that is virNetDevMacVLanTapOpen() returns a negative value, we unroll all the changes BUT return 0 fooling caller into thinking everything went okay.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c05c67f..bd339c0 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -1120,8 +1120,10 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, }
if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { - if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) + if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) { + VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
I know this is removed in the next patch, but closing fd 0 is just wrong here. ACK if you change it to just: rc = -1; Jan
This variable is very misleading. We use VIR_FORCE_CLOSE to set it to -1 and returning it even though it does not refer to a FD at all. It merely holds 0 or -1. Drop it completely. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index bd339c0..95ab447 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -989,7 +989,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; - int rc, reservedID = -1; + int reservedID = -1; char ifname[IFNAMSIZ]; int retries, do_retry = 0; uint32_t macvtapMode; @@ -1079,9 +1079,8 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, return -1; } snprintf(ifname, sizeof(ifname), pattern, reservedID); - rc = virNetDevMacVLanCreate(ifname, type, macaddress, linkdev, - macvtapMode, &do_retry); - if (rc < 0) { + if (virNetDevMacVLanCreate(ifname, type, macaddress, linkdev, + macvtapMode, &do_retry) < 0) { virNetDevMacVLanReleaseID(reservedID, flags); virMutexUnlock(&virNetDevMacVLanCreateMutex); if (!do_retry) @@ -1107,36 +1106,26 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, macaddress, linkdev, vf, - vmuuid, vmOp, false) < 0) { - rc = -1; + vmuuid, vmOp, false) < 0) goto link_del_exit; - } if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) { - if (virNetDevSetOnline(ifnameCreated, true) < 0) { - rc = -1; + if (virNetDevSetOnline(ifnameCreated, true) < 0) goto disassociate_exit; - } } if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { - if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) { - VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ + if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) goto disassociate_exit; - } - if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr) < 0) { - VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ + if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr) < 0) goto disassociate_exit; - } - if (VIR_STRDUP(*ifnameResult, ifnameCreated) < 0) { - VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ + + if (VIR_STRDUP(*ifnameResult, ifnameCreated) < 0) goto disassociate_exit; - } } else { if (VIR_STRDUP(*ifnameResult, ifnameCreated) < 0) goto disassociate_exit; - rc = 0; } if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || @@ -1149,10 +1138,10 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, linkdev, vmuuid, virtPortProfile, vmOp) < 0) - goto disassociate_exit; + goto disassociate_exit; } - return rc; + return 0; disassociate_exit: ignore_value(virNetDevVPortProfileDisassociate(ifnameCreated, @@ -1168,7 +1157,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, ignore_value(virNetDevMacVLanDelete(ifnameCreated)); virNetDevMacVLanReleaseName(ifnameCreated); - return rc; + return -1; } -- 2.8.4
On Tue, Aug 09, 2016 at 07:32:44PM +0200, Michal Privoznik wrote:
This variable is very misleading. We use VIR_FORCE_CLOSE to set
The function is also hard to follow.
it to -1 and returning it even though it does not refer to a FD at all. It merely holds 0 or -1. Drop it completely.
Yes, introduced by commit 136fe2f7: virNetDevMacVLanTapSetup: Rework to support multiple FDs
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 35 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 23 deletions(-)
@@ -1079,9 +1079,8 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, return -1; } snprintf(ifname, sizeof(ifname), pattern, reservedID); - rc = virNetDevMacVLanCreate(ifname, type, macaddress, linkdev, - macvtapMode, &do_retry); - if (rc < 0) { + if (virNetDevMacVLanCreate(ifname, type, macaddress, linkdev, + macvtapMode, &do_retry) < 0) { virNetDevMacVLanReleaseID(reservedID, flags); virMutexUnlock(&virNetDevMacVLanCreateMutex); if (!do_retry) @@ -1107,36 +1106,26 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, macaddress, linkdev, vf, - vmuuid, vmOp, false) < 0) { - rc = -1; + vmuuid, vmOp, false) < 0) goto link_del_exit; - }
if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) { - if (virNetDevSetOnline(ifnameCreated, true) < 0) { - rc = -1; + if (virNetDevSetOnline(ifnameCreated, true) < 0) goto disassociate_exit; - } }
if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { - if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) { - VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ + if (virNetDevMacVLanTapOpen(ifnameCreated, tapfd, tapfdSize, 10) < 0) goto disassociate_exit; - }
- if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr) < 0) { - VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ + if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr) < 0) goto disassociate_exit; - } - if (VIR_STRDUP(*ifnameResult, ifnameCreated) < 0) { - VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ + + if (VIR_STRDUP(*ifnameResult, ifnameCreated) < 0) goto disassociate_exit; - }
All the gotos above are preceded by rc = -1, no functional change there.
} else { if (VIR_STRDUP(*ifnameResult, ifnameCreated) < 0) goto disassociate_exit;
Before, if the if (ifnameRequested) block gets skipped because the requested name matches the automatic prefix, rc is 0 here, otherwise it's uninitialized. And we would return it on the unlikely OOM error..
- rc = 0; }
if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE || @@ -1149,10 +1138,10 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, linkdev, vmuuid, virtPortProfile, vmOp) < 0) - goto disassociate_exit; + goto disassociate_exit;
..and here too, even though we delete the device. This patch fixes it. ACK, it might be nice to mention in the commit message that this also fixes the return value in some corner cases. Jan
}
- return rc; + return 0;
Usually, this variable is used to hold the return value for a function of ours. Well, this is not the case. Its use does not match our pattern and therefore it is very misleading. Drop it and define an alternative @rc variable, but only in that single block where it is needed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 95ab447..c8c16b5 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -994,7 +994,6 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, int retries, do_retry = 0; uint32_t macvtapMode; const char *ifnameCreated = NULL; - int ret; int vf = -1; bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR; @@ -1028,6 +1027,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, } if (ifnameRequested) { + int rc; bool isAutoName = (STRPREFIX(ifnameRequested, MACVTAP_NAME_PREFIX) || STRPREFIX(ifnameRequested, MACVLAN_NAME_PREFIX)); @@ -1035,11 +1035,11 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, VIR_INFO("Requested macvtap device name: %s", ifnameRequested); virMutexLock(&virNetDevMacVLanCreateMutex); - if ((ret = virNetDevExists(ifnameRequested)) < 0) { + if ((rc = virNetDevExists(ifnameRequested)) < 0) { virMutexUnlock(&virNetDevMacVLanCreateMutex); return -1; } - if (ret) { + if (rc) { if (isAutoName) goto create_name; virReportSystemError(EEXIST, -- 2.8.4
On Tue, Aug 09, 2016 at 07:32:45PM +0200, Michal Privoznik wrote:
Usually, this variable is used to hold the return value for a function of ours. Well, this is not the case. Its use does not match our pattern and therefore it is very misleading. Drop it and define an alternative @rc variable, but only in that single block where it is needed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK Jan
participants (2)
-
Ján Tomko -
Michal Privoznik