1:32 p.m.
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
1:32 p.m.
New subject: [libvirt] [PATCH 1/3] virNetDevMacVLanCreateWithVPortProfile: Don't mask virNetDevMacVLanTapOpen error
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(a)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
10:04 a.m.
New subject: [libvirt] [PATCH 1/3] virNetDevMacVLanCreateWithVPortProfile: Don't mask virNetDevMacVLanTapOpen error
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(a)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
1:32 p.m.
New subject: [libvirt] [PATCH 2/3] virNetDevMacVLanCreateWithVPortProfile: Drop @rc
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(a)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
10:28 a.m.
New subject: [libvirt] [PATCH 2/3] virNetDevMacVLanCreateWithVPortProfile: Drop @rc
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(a)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;
1:32 p.m.
New subject: [libvirt] [PATCH 3/3] virNetDevMacVLanCreateWithVPortProfile: Drop @ret
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(a)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
10:30 a.m.
New subject: [libvirt] [PATCH 3/3] virNetDevMacVLanCreateWithVPortProfile: Drop @ret
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(a)redhat.com>
---
src/util/virnetdevmacvlan.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
ACK
Jan
3223
days inactive
3224
days old
6 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik