[libvirt] [PATCH] macvtap: Fix error return values to -1 instead of 1

[Roopa Prabhu]

On 10/20/11 11:57 AM, "Laine Stump" <laine at laine.org> wrote:

> On 10/20/2011 01:46 PM, Roopa Prabhu wrote:
>> From: Roopa Prabhu<roprabhu at 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 at cisco.com>
>> Reported-by: Eric Blake<eblake at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list