12:46 p.m.
From: Roopa Prabhu <roprabhu(a)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(a)cisco.com>
Reported-by: Eric Blake <eblake(a)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:
1:57 p.m.
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu(a)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(a)cisco.com>
Reported-by: Eric Blake<eblake(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:18 p.m.
On 10/20/11 11:57 AM, "Laine Stump" <laine(a)laine.org> wrote:
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
> From: Roopa Prabhu<roprabhu(a)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(a)cisco.com>
> Reported-by: Eric Blake<eblake(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1:14 p.m.
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
From: Roopa Prabhu<roprabhu(a)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(a)cisco.com>
Reported-by: Eric Blake<eblake(a)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
10:01 p.m.
On 10/24/11 11:14 AM, "Stefan Berger" <stefanb(a)linux.vnet.ibm.com> wrote:
On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
> From: Roopa Prabhu<roprabhu(a)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(a)cisco.com>
> Reported-by: Eric Blake<eblake(a)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(a)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
6:57 p.m.
On 10/26/11 8:01 PM, "Roopa Prabhu" <roprabhu(a)cisco.com> wrote:
On 10/24/11 11:14 AM, "Stefan Berger" <stefanb(a)linux.vnet.ibm.com>
wrote:
> On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
>> From: Roopa Prabhu<roprabhu(a)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(a)cisco.com>
>> Reported-by: Eric Blake<eblake(a)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(a)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.
9:01 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:10 p.m.
On 10/27/11 7:01 PM, "Eric Blake" <eblake(a)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.
4807
days inactive
4815
days old
7 comments
4 participants
participants (4)
-
Eric Blake -
Laine Stump -
Roopa Prabhu -
Stefan Berger