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

On 10/20/2011 01:46 PM, Roopa Prabhu wrote: > From: Roopa Prabhu&lt;roprabhu(a)cisco.com&gt; > > 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&lt;roprabhu(a)cisco.com&gt; > Reported-by: Eric Blake&lt;eblake(a)redhat.com&gt; > --- > 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