Re: [PATCH 2/3] virNetDevMacVLanCreateWithVPortProfile: Drop @rc

[Ján Tomko <jtomko@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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;

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list