[libvirt] [PATCH] macvtap: Fix error return values to -1 instead of 1
From: Roopa Prabhu <roprabhu@cisco.com> Fixes some cases where 1 was being returned instead of -1. There are still some inconsistencies in the file with respect to what the return variable is initialized to. Can be fixed as a separate patch if needed. The scope of this patch is just to fix the return value 1. Did some basic sanity test. Signed-off-by: Roopa Prabhu <roprabhu@cisco.com> Reported-by: Eric Blake <eblake@redhat.com> --- src/util/macvtap.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 7fd6eb5..f8b9d55 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -480,7 +480,7 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf, bool is8021Qbg, uint16_t *status) { - int rc = 1; + int rc = -1; const char *msg = NULL; struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, }; @@ -806,7 +806,7 @@ doPortProfileOpCommon(bool nltarget_kernel, _("error %d during port-profile setlink on " "interface %s (%d)"), status, ifname, ifindex); - rc = 1; + rc = -1; break; } @@ -867,7 +867,7 @@ doPortProfileOp8021Qbg(const char *ifname, const virVirtualPortProfileParamsPtr virtPort, enum virVirtualPortOp virtPortOp) { - int rc; + int rc = -1; # ifndef IFLA_VF_PORT_MAX @@ -877,7 +877,6 @@ doPortProfileOp8021Qbg(const char *ifname, (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); - rc = 1; # else /* IFLA_VF_PORT_MAX */ @@ -893,10 +892,8 @@ doPortProfileOp8021Qbg(const char *ifname, int vf = PORT_SELF_VF; if (getPhysdevAndVlan(ifname, &physdev_ifindex, physdev_ifname, - &vlanid) != 0) { - rc = 1; + &vlanid) != 0) goto err_exit; - } if (vlanid < 0) vlanid = 0; @@ -918,7 +915,6 @@ doPortProfileOp8021Qbg(const char *ifname, default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); - rc = 1; goto err_exit; } @@ -982,7 +978,7 @@ doPortProfileOp8021Qbh(const char *ifname, const unsigned char *vm_uuid, enum virVirtualPortOp virtPortOp) { - int rc; + int rc = -1; # ifndef IFLA_VF_PORT_MAX @@ -993,7 +989,6 @@ doPortProfileOp8021Qbh(const char *ifname, (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); - rc = 1; # else /* IFLA_VF_PORT_MAX */ @@ -1008,10 +1003,9 @@ doPortProfileOp8021Qbh(const char *ifname, if (rc) goto err_exit; - if (ifaceGetIndex(true, physfndev, &ifindex) < 0) { - rc = 1; + rc = ifaceGetIndex(true, physfndev, &ifindex); + if (rc < 0) goto err_exit; - } switch (virtPortOp) { case PREASSOCIATE_RR: @@ -1059,7 +1053,7 @@ doPortProfileOp8021Qbh(const char *ifname, default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); - rc = 1; + rc = -1; } err_exit:
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu@cisco.com>
Fixes some cases where 1 was being returned instead of -1. There are still some inconsistencies in the file with respect to what the return variable is initialized to. Can be fixed as a separate patch if needed. The scope of this patch is just to fix the return value 1. Did some basic sanity test.
This patch hasn't changed the checks of the return code made by callers to these functions - a spot check showed several that still say "if (rc) { ...." rather than "if (rc < 0) { ....". While that is technically correct, it is inconsistent with the preferred style in libvirt, and I'm guessing that style is at least partly the reason for making this patch in the first place :-). As long as you're going to make this change, you may as well trace back up the call chain for all changed functions and fix the callers to be consistent. (I also noticed at least one place that uses "xxx() != 0" instead of "xxx() < 0". Making all of these comparisons consistent will make it much easier for someone auditing the code in the future to understand that the "errors are < 0" convention has been followed for these functions.)
Signed-off-by: Roopa Prabhu<roprabhu@cisco.com> Reported-by: Eric Blake<eblake@redhat.com> --- src/util/macvtap.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 7fd6eb5..f8b9d55 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -480,7 +480,7 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf, bool is8021Qbg, uint16_t *status) { - int rc = 1; + int rc = -1; const char *msg = NULL; struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };
@@ -806,7 +806,7 @@ doPortProfileOpCommon(bool nltarget_kernel, _("error %d during port-profile setlink on " "interface %s (%d)"), status, ifname, ifindex); - rc = 1; + rc = -1; break; }
@@ -867,7 +867,7 @@ doPortProfileOp8021Qbg(const char *ifname, const virVirtualPortProfileParamsPtr virtPort, enum virVirtualPortOp virtPortOp) { - int rc; + int rc = -1;
# ifndef IFLA_VF_PORT_MAX
@@ -877,7 +877,6 @@ doPortProfileOp8021Qbg(const char *ifname, (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); - rc = 1;
# else /* IFLA_VF_PORT_MAX */
@@ -893,10 +892,8 @@ doPortProfileOp8021Qbg(const char *ifname, int vf = PORT_SELF_VF;
if (getPhysdevAndVlan(ifname,&physdev_ifindex, physdev_ifname, -&vlanid) != 0) { - rc = 1; +&vlanid) != 0) goto err_exit; - }
if (vlanid< 0) vlanid = 0; @@ -918,7 +915,6 @@ doPortProfileOp8021Qbg(const char *ifname, default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); - rc = 1; goto err_exit; }
@@ -982,7 +978,7 @@ doPortProfileOp8021Qbh(const char *ifname, const unsigned char *vm_uuid, enum virVirtualPortOp virtPortOp) { - int rc; + int rc = -1;
# ifndef IFLA_VF_PORT_MAX
@@ -993,7 +989,6 @@ doPortProfileOp8021Qbh(const char *ifname, (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); - rc = 1;
# else /* IFLA_VF_PORT_MAX */
@@ -1008,10 +1003,9 @@ doPortProfileOp8021Qbh(const char *ifname, if (rc) goto err_exit;
- if (ifaceGetIndex(true, physfndev,&ifindex)< 0) { - rc = 1; + rc = ifaceGetIndex(true, physfndev,&ifindex); + if (rc< 0) goto err_exit; - }
switch (virtPortOp) { case PREASSOCIATE_RR: @@ -1059,7 +1053,7 @@ doPortProfileOp8021Qbh(const char *ifname, default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); - rc = 1; + rc = -1; }
err_exit:
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/20/11 11:57 AM, "Laine Stump" <laine@laine.org> wrote:
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu@cisco.com>
Fixes some cases where 1 was being returned instead of -1. There are still some inconsistencies in the file with respect to what the return variable is initialized to. Can be fixed as a separate patch if needed. The scope of this patch is just to fix the return value 1. Did some basic sanity test.
This patch hasn't changed the checks of the return code made by callers to these functions - a spot check showed several that still say "if (rc) { ...." rather than "if (rc < 0) { ....". While that is technically correct, it is inconsistent with the preferred style in libvirt, and I'm guessing that style is at least partly the reason for making this patch in the first place :-). As long as you're going to make this change, you may as well trace back up the call chain for all changed functions and fix the callers to be consistent.
Actually I did trace the call chain for every change and I thought if (rc) for negative values was just fine. Dint realize that libvirt preferred style was if (rc < 0)
(I also noticed at least one place that uses "xxx() != 0" instead of "xxx() < 0". Making all of these comparisons consistent will make it much easier for someone auditing the code in the future to understand that the "errors are < 0" convention has been followed for these functions.)
Yes I do agree that this file has some inconsistency in the checks. I noticed that and also listed one of them in the log comment for this patch. And only removed the return 1. Agree that its good to fix all inconsistencies, I will fix them all and respin. Thanks! Roopa
Signed-off-by: Roopa Prabhu<roprabhu@cisco.com> Reported-by: Eric Blake<eblake@redhat.com> --- src/util/macvtap.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 7fd6eb5..f8b9d55 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -480,7 +480,7 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf, bool is8021Qbg, uint16_t *status) { - int rc = 1; + int rc = -1; const char *msg = NULL; struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };
@@ -806,7 +806,7 @@ doPortProfileOpCommon(bool nltarget_kernel, _("error %d during port-profile setlink on " "interface %s (%d)"), status, ifname, ifindex); - rc = 1; + rc = -1; break; }
@@ -867,7 +867,7 @@ doPortProfileOp8021Qbg(const char *ifname, const virVirtualPortProfileParamsPtr virtPort, enum virVirtualPortOp virtPortOp) { - int rc; + int rc = -1;
# ifndef IFLA_VF_PORT_MAX
@@ -877,7 +877,6 @@ doPortProfileOp8021Qbg(const char *ifname, (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); - rc = 1;
# else /* IFLA_VF_PORT_MAX */
@@ -893,10 +892,8 @@ doPortProfileOp8021Qbg(const char *ifname, int vf = PORT_SELF_VF;
if (getPhysdevAndVlan(ifname,&physdev_ifindex, physdev_ifname, -&vlanid) != 0) { - rc = 1; +&vlanid) != 0) goto err_exit; - }
if (vlanid< 0) vlanid = 0; @@ -918,7 +915,6 @@ doPortProfileOp8021Qbg(const char *ifname, default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); - rc = 1; goto err_exit; }
@@ -982,7 +978,7 @@ doPortProfileOp8021Qbh(const char *ifname, const unsigned char *vm_uuid, enum virVirtualPortOp virtPortOp) { - int rc; + int rc = -1;
# ifndef IFLA_VF_PORT_MAX
@@ -993,7 +989,6 @@ doPortProfileOp8021Qbh(const char *ifname, (void)virtPortOp; macvtapError(VIR_ERR_INTERNAL_ERROR, "%s", _("Kernel VF Port support was missing at compile time.")); - rc = 1;
# else /* IFLA_VF_PORT_MAX */
@@ -1008,10 +1003,9 @@ doPortProfileOp8021Qbh(const char *ifname, if (rc) goto err_exit;
- if (ifaceGetIndex(true, physfndev,&ifindex)< 0) { - rc = 1; + rc = ifaceGetIndex(true, physfndev,&ifindex); + if (rc< 0) goto err_exit; - }
switch (virtPortOp) { case PREASSOCIATE_RR: @@ -1059,7 +1053,7 @@ doPortProfileOp8021Qbh(const char *ifname, default: macvtapError(VIR_ERR_INTERNAL_ERROR, _("operation type %d not supported"), virtPortOp); - rc = 1; + rc = -1; }
err_exit:
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu@cisco.com>
Fixes some cases where 1 was being returned instead of -1. There are still some inconsistencies in the file with respect to what the return variable is initialized to. Can be fixed as a separate patch if needed. The scope of this patch is just to fix the return value 1. Did some basic sanity test.
Signed-off-by: Roopa Prabhu<roprabhu@cisco.com> Reported-by: Eric Blake<eblake@redhat.com> --- src/util/macvtap.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 7fd6eb5..f8b9d55 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -480,7 +480,7 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf, bool is8021Qbg, uint16_t *status) { - int rc = 1; + int rc = -1; const char *msg = NULL; struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };
@@ -806,7 +806,7 @@ doPortProfileOpCommon(bool nltarget_kernel, _("error %d during port-profile setlink on " "interface %s (%d)"), status, ifname, ifindex); - rc = 1; + rc = -1; break; }
In this function we later on return a -ETIMEDOUT. The -1 doesn't overlap with it, but I am wondering whether we should return -EFAULT in the places of -1 now ? Stefan
On 10/24/11 11:14 AM, "Stefan Berger" <stefanb@linux.vnet.ibm.com> wrote:
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu@cisco.com>
Fixes some cases where 1 was being returned instead of -1. There are still some inconsistencies in the file with respect to what the return variable is initialized to. Can be fixed as a separate patch if needed. The scope of this patch is just to fix the return value 1. Did some basic sanity test.
Signed-off-by: Roopa Prabhu<roprabhu@cisco.com> Reported-by: Eric Blake<eblake@redhat.com> --- src/util/macvtap.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 7fd6eb5..f8b9d55 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -480,7 +480,7 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf, bool is8021Qbg, uint16_t *status) { - int rc = 1; + int rc = -1; const char *msg = NULL; struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };
@@ -806,7 +806,7 @@ sassmann@redhat.com(bool nltarget_kernel, _("error %d during port-profile setlink on " "interface %s (%d)"), status, ifname, ifindex); - rc = 1; + rc = -1; break; }
In this function we later on return a -ETIMEDOUT. The -1 doesn't overlap with it, but I am wondering whether we should return -EFAULT in the places of -1 now ?
Instead of defaulting to -EFAULT, shall I just add a function to map the port profile/VDP status codes to closest errno (with default being unknown error) and use that instead ?. Also, for some reason we are reporting EINVAL in the virReportSystemError just above it. EINVAL also does not seem right there. I can fix it. Thanks, Roopa
On 10/26/11 8:01 PM, "Roopa Prabhu" <roprabhu@cisco.com> wrote:
On 10/24/11 11:14 AM, "Stefan Berger" <stefanb@linux.vnet.ibm.com> wrote:
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu@cisco.com>
Fixes some cases where 1 was being returned instead of -1. There are still some inconsistencies in the file with respect to what the return variable is initialized to. Can be fixed as a separate patch if needed. The scope of this patch is just to fix the return value 1. Did some basic sanity test.
Signed-off-by: Roopa Prabhu<roprabhu@cisco.com> Reported-by: Eric Blake<eblake@redhat.com> --- src/util/macvtap.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 7fd6eb5..f8b9d55 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -480,7 +480,7 @@ getPortProfileStatus(struct nlattr **tb, int32_t vf, bool is8021Qbg, uint16_t *status) { - int rc = 1; + int rc = -1; const char *msg = NULL; struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };
@@ -806,7 +806,7 @@ sassmann@redhat.com(bool nltarget_kernel, _("error %d during port-profile setlink on " "interface %s (%d)"), status, ifname, ifindex); - rc = 1; + rc = -1; break; }
In this function we later on return a -ETIMEDOUT. The -1 doesn't overlap with it, but I am wondering whether we should return -EFAULT in the places of -1 now ?
Instead of defaulting to -EFAULT, shall I just add a function to map the port profile/VDP status codes to closest errno (with default being unknown error) and use that instead ?. Also, for some reason we are reporting EINVAL in the virReportSystemError just above it. EINVAL also does not seem right there.
I can fix it. Thanks, Roopa
Also, looks like no where else in libvirt code we return errno. We only print errno but always return -1. And the -ETIMEDOUT case in macvtap is an exception, cause the upper layer requires the cause of this particular error. I don't mind changing everything to errno. But that seems to be not the convention. And I cant find a clean way to not return -ETIMEDOUT. I can however return status of the command in an argument and return -1 for the ETIMEDOUT case. I will do that unless you have other suggestions. Thanks.
On 10/27/2011 05:57 PM, Roopa Prabhu wrote:
Also, looks like no where else in libvirt code we return errno. We only print errno but always return -1. And the -ETIMEDOUT case in macvtap is an exception, cause the upper layer requires the cause of this particular error.
We have a few places that return -1 on most errors, 0 on normal success, and -2 on timeout; that is, the < 0 check can filter out all non-success, but callers that care about the particular failure can compare against -1. If all errors can map to errno, we do have some examples of API that return -errno; but those tend to be lower level API (lots in util/util.c, not so many elsewhere); it tends to be easier to return -1 and document if errno is set to a sane value on failure.
I don't mind changing everything to errno. But that seems to be not the convention. And I cant find a clean way to not return -ETIMEDOUT. I can however return status of the command in an argument and return -1 for the ETIMEDOUT case. I will do that unless you have other suggestions. Thanks.
Returning -1 and -errno in the same function doesn't work (since -1 would be ambiguous with EACCES, on some systems), but returning -1 vs. -2 to distinguish timeout from normal errors is fine. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/27/11 7:01 PM, "Eric Blake" <eblake@redhat.com> wrote:
On 10/27/2011 05:57 PM, Roopa Prabhu wrote:
Also, looks like no where else in libvirt code we return errno. We only print errno but always return -1. And the -ETIMEDOUT case in macvtap is an exception, cause the upper layer requires the cause of this particular error.
We have a few places that return -1 on most errors, 0 on normal success, and -2 on timeout; that is, the < 0 check can filter out all non-success, but callers that care about the particular failure can compare against -1.
If all errors can map to errno, we do have some examples of API that return -errno; but those tend to be lower level API (lots in util/util.c, not so many elsewhere); it tends to be easier to return -1 and document if errno is set to a sane value on failure.
I don't mind changing everything to errno. But that seems to be not the convention. And I cant find a clean way to not return -ETIMEDOUT. I can however return status of the command in an argument and return -1 for the ETIMEDOUT case. I will do that unless you have other suggestions. Thanks.
Returning -1 and -errno in the same function doesn't work (since -1 would be ambiguous with EACCES, on some systems), but returning -1 vs. -2 to distinguish timeout from normal errors is fine.
Ok thanks eric. This helps.
participants (4)
-
Eric Blake -
Laine Stump -
Roopa Prabhu -
Stefan Berger