util: standardize return from functions calling virNetlinkCommand

There are several functions that call virNetlinkCommand, and they all
follow a common pattern, with three exit labels: err_exit (or
cleanup), malformed_resp, and buffer_too_small. All three of these
labels do their own cleanup and have their own return. However, the
malformed_resp label usually frees the same items as the
cleanup/err_exit label, and the buffer_too_small label just doesn't
free recvbuf (because it's known to always be NULL at the time we goto
buffer_too_small.

In order to simplify and standardize the code, I've made the following
changes to all of these functions:

1) err_exit is replaced with the more libvirt-ish "cleanup", which
   makes sense because in all cases this code is also executed in the
   case of success, so labelling it err_exit may be confusing.

2) rc is initialized to -1, and set to 0 just before the cleanup
   label. Any code that currently sets rc = -1 is made to instead goto
   cleanup.

3) malformed_resp and buffer_too_small just log their error and goto
   cleanup. This gives us a single return path, and a single place to
   free up resources.

4) In one instance, rather then logging an error immediately, a char*
   msg was pointed to an error string, then goto cleanup (and cleanup
   would log an error if msg != NULL). It takes no more lines of code
   to just log the message as we encounter it.

This patch should have 0 functional effects.

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2cc699a54a03e19777142ababe9d55ecc732003f..6eec5cf2a08347d0a963dabcfbe97b3eaf3b6fa9 100644 (file)
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1249,7 +1249,7 @@ virNetDevLinkDump(const char *ifname, int ifindex,
                   unsigned char **recvbuf,
                   uint32_t (*getPidFunc)(void))
 {
-    int rc = 0;
+    int rc = -1;
     struct nlmsghdr *resp;
     struct nlmsgerr *err;
     struct ifinfomsg ifinfo = {
@@ -1284,15 +1284,12 @@ virNetDevLinkDump(const char *ifname, int ifindex,
     if (!nltarget_kernel) {
         pid = getPidFunc();
         if (pid == 0) {
-            rc = -1;
             goto cleanup;
         }
     }
 
-    if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, pid) < 0) {
-        rc = -1;
+    if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, pid) < 0)
         goto cleanup;
-    }
 
     if (recvbuflen < NLMSG_LENGTH(0) || *recvbuf == NULL)
         goto malformed_resp;
@@ -1309,7 +1306,7 @@ virNetDevLinkDump(const char *ifname, int ifindex,
             virReportSystemError(-err->error,
                                  _("error dumping %s (%d) interface"),
                                  ifname, ifindex);
-            rc = -1;
+            goto cleanup;
         }
         break;
 
@@ -1324,29 +1321,22 @@ virNetDevLinkDump(const char *ifname, int ifindex,
     default:
         goto malformed_resp;
     }
-
-    if (rc != 0)
-        VIR_FREE(*recvbuf);
-
+    rc = 0;
 cleanup:
+    if (rc < 0)
+        VIR_FREE(*recvbuf);
     nlmsg_free(nl_msg);
-
     return rc;
 
 malformed_resp:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("malformed netlink response message"));
-    VIR_FREE(*recvbuf);
-    return -1;
+    goto cleanup;
 
 buffer_too_small:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("allocated netlink buffer is too small"));
-    return -1;
+    goto cleanup;
 }
 
 static int
@@ -1457,28 +1447,20 @@ virNetDevSetVfConfig(const char *ifname, int ifindex, int vf,
     }
 
     rc = 0;
-
 cleanup:
     nlmsg_free(nl_msg);
-
     VIR_FREE(recvbuf);
-
     return rc;
 
 malformed_resp:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("malformed netlink response message"));
-    VIR_FREE(recvbuf);
-    return rc;
+    goto cleanup;
 
 buffer_too_small:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("allocated netlink buffer is too small"));
-    return rc;
+    goto cleanup;
 }
 
 static int

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 647679f6f8e286c155f86d170f1f2afbb1ed5f2a..d467990cfe9f46aa6c215d68ac9c5959211ab0c9 100644 (file)
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -100,7 +100,7 @@ virNetDevMacVLanCreate(const char *ifname,
                        uint32_t macvlan_mode,
                        int *retry)
 {
-    int rc = 0;
+    int rc = -1;
     struct nlmsghdr *resp;
     struct nlmsgerr *err;
     struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
@@ -155,7 +155,6 @@ virNetDevMacVLanCreate(const char *ifname,
     nla_nest_end(nl_msg, linkinfo);
 
     if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0) < 0) {
-        rc = -1;
         goto cleanup;
     }
 
@@ -177,14 +176,13 @@ virNetDevMacVLanCreate(const char *ifname,
 
         case -EEXIST:
             *retry = 1;
-            rc = -1;
-            break;
+            goto cleanup;
 
         default:
             virReportSystemError(-err->error,
                                  _("error creating %s type of interface"),
                                  type);
-            rc = -1;
+            goto cleanup;
         }
         break;
 
@@ -195,27 +193,21 @@ virNetDevMacVLanCreate(const char *ifname,
         goto malformed_resp;
     }
 
+    rc = 0;
 cleanup:
     nlmsg_free(nl_msg);
-
     VIR_FREE(recvbuf);
-
     return rc;
 
 malformed_resp:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("malformed netlink response message"));
-    VIR_FREE(recvbuf);
-    return -1;
+    goto cleanup;
 
 buffer_too_small:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("allocated netlink buffer is too small"));
-    return -1;
+    goto cleanup;
 }
 
 /**
@@ -229,7 +221,7 @@ buffer_too_small:
  */
 int virNetDevMacVLanDelete(const char *ifname)
 {
-    int rc = 0;
+    int rc = -1;
     struct nlmsghdr *resp;
     struct nlmsgerr *err;
     struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
@@ -251,7 +243,6 @@ int virNetDevMacVLanDelete(const char *ifname)
         goto buffer_too_small;
 
     if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0) < 0) {
-        rc = -1;
         goto cleanup;
     }
 
@@ -270,7 +261,7 @@ int virNetDevMacVLanDelete(const char *ifname)
             virReportSystemError(-err->error,
                                  _("error destroying %s interface"),
                                  ifname);
-            rc = -1;
+            goto cleanup;
         }
         break;
 
@@ -281,27 +272,21 @@ int virNetDevMacVLanDelete(const char *ifname)
         goto malformed_resp;
     }
 
+    rc = 0;
 cleanup:
     nlmsg_free(nl_msg);
-
     VIR_FREE(recvbuf);
-
     return rc;
 
 malformed_resp:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("malformed netlink response message"));
-    VIR_FREE(recvbuf);
-    return -1;
+    goto cleanup;
 
 buffer_too_small:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("allocated netlink buffer is too small"));
-    return -1;
+    goto cleanup;
 }
 
 

diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
index bd356d82efe4f346b4ba08e4d4dee61a4f6c713d..5b562be44bfb6805e516e9a048afd6c3fbb2e2b1 100644 (file)
--- a/src/util/virnetdevvportprofile.c
+++ b/src/util/virnetdevvportprofile.c
@@ -179,19 +179,20 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
                                uint16_t *status)
 {
     int rc = -1;
-    const char *msg = NULL;
     struct nlattr *tb_port[IFLA_PORT_MAX + 1] = { NULL, };
 
     if (vf == PORT_SELF_VF && nltarget_kernel) {
         if (tb[IFLA_PORT_SELF]) {
             if (nla_parse_nested(tb_port, IFLA_PORT_MAX, tb[IFLA_PORT_SELF],
                                  ifla_port_policy)) {
-                msg = _("error parsing IFLA_PORT_SELF part");
-                goto err_exit;
+                virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("error parsing IFLA_PORT_SELF part"));
+                goto cleanup;
             }
         } else {
-            msg = _("IFLA_PORT_SELF is missing");
-            goto err_exit;
+            virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("IFLA_PORT_SELF is missing"));
+            goto cleanup;
         }
     } else {
         if (tb[IFLA_VF_PORTS]) {
@@ -202,14 +203,17 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
             nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) {
 
                 if (nla_type(tb_vf_ports) != IFLA_VF_PORT) {
-                    msg = _("error while iterating over IFLA_VF_PORTS part");
-                    goto err_exit;
+                    virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+                                   _("error while iterating over "
+                                     "IFLA_VF_PORTS part"));
+                    goto cleanup;
                 }
 
                 if (nla_parse_nested(tb_port, IFLA_PORT_MAX, tb_vf_ports,
                                      ifla_port_policy)) {
-                    msg = _("error parsing IFLA_VF_PORT part");
-                    goto err_exit;
+                    virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+                                   _("error parsing IFLA_VF_PORT part"));
+                    goto cleanup;
                 }
 
                 if (instanceId &&
@@ -226,13 +230,15 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
             }
 
             if (!found) {
-                msg = _("Could not find netlink response with "
-                        "expected parameters");
-                goto err_exit;
+                virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("Could not find netlink response with "
+                                 "expected parameters"));
+                goto cleanup;
             }
         } else {
-            msg = _("IFLA_VF_PORTS is missing");
-            goto err_exit;
+            virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("IFLA_VF_PORTS is missing"));
+            goto cleanup;
         }
     }
 
@@ -245,15 +251,12 @@ virNetDevVPortProfileGetStatus(struct nlattr **tb, int32_t vf,
             *status = PORT_PROFILE_RESPONSE_INPROGRESS;
             rc = 0;
         } else {
-            msg = _("no IFLA_PORT_RESPONSE found in netlink message");
-            goto err_exit;
+            virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("no IFLA_PORT_RESPONSE found in netlink message"));
+            goto cleanup;
         }
     }
-
-err_exit:
-    if (msg)
-        virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s", msg);
-
+cleanup:
     return rc;
 }
 
@@ -389,11 +392,11 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
     if (!nltarget_kernel) {
         pid = virNetDevVPortProfileGetLldpadPid();
         if (pid == 0)
-            goto err_exit;
+            goto cleanup;
     }
 
     if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, pid) < 0)
-        goto err_exit;
+        goto cleanup;
 
     if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL)
         goto malformed_resp;
@@ -410,7 +413,7 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
             virReportSystemError(-err->error,
                 _("error during virtual port configuration of ifindex %d"),
                 ifindex);
-            goto err_exit;
+            goto cleanup;
         }
         break;
 
@@ -422,28 +425,20 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex,
     }
 
     rc = 0;
-
-err_exit:
+cleanup:
     nlmsg_free(nl_msg);
-
     VIR_FREE(recvbuf);
-
     return rc;
 
 malformed_resp:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("malformed netlink response message"));
-    VIR_FREE(recvbuf);
-    return rc;
+    goto cleanup;
 
 buffer_too_small:
-    nlmsg_free(nl_msg);
-
     virNetDevError(VIR_ERR_INTERNAL_ERROR, "%s",
                    _("allocated netlink buffer is too small"));
-    return rc;
+    goto cleanup;
 }
 
 
@@ -558,12 +553,12 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
         rc = virNetDevLinkDump(NULL, ifindex, nltarget_kernel, tb,
                                &recvbuf, virNetDevVPortProfileGetLldpadPid);
         if (rc < 0)
-            goto err_exit;
+            goto cleanup;
 
         rc = virNetDevVPortProfileGetStatus(tb, vf, instanceId, nltarget_kernel,
                                             is8021Qbg, &status);
         if (rc < 0)
-            goto err_exit;
+            goto cleanup;
         if (status == PORT_PROFILE_RESPONSE_SUCCESS ||
             status == PORT_VDP_RESPONSE_SUCCESS) {
             break;
@@ -589,7 +584,7 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
         rc = -2;
     }
 
-err_exit:
+cleanup:
     VIR_FREE(recvbuf);
 
     return rc;
@@ -634,7 +629,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
                                enum virNetDevVPortProfileLinkOp virtPortOp,
                                bool setlink_only)
 {
-    int rc = 0;
+    int rc = -1;
     int op = PORT_REQUEST_ASSOCIATE;
     struct ifla_port_vsi portVsi = {
         .vsi_mgr_id       = virtPort->u.virtPort8021Qbg.managerID,
@@ -650,10 +645,9 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
 
     vf = PORT_SELF_VF;
 
-    if (virNetDevVPortProfileGetPhysdevAndVlan(ifname, &physdev_ifindex, physdev_ifname,
-                                               &vlanid) < 0) {
-        rc = -1;
-        goto err_exit;
+    if (virNetDevVPortProfileGetPhysdevAndVlan(ifname, &physdev_ifindex,
+                                               physdev_ifname, &vlanid) < 0) {
+        goto cleanup;
     }
 
     if (vlanid < 0)
@@ -676,8 +670,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
     default:
         virNetDevError(VIR_ERR_INTERNAL_ERROR,
                        _("operation type %d not supported"), virtPortOp);
-        rc = -1;
-        goto err_exit;
+        goto cleanup;
     }
 
     rc = virNetDevVPortProfileOpCommon(physdev_ifname, physdev_ifindex,
@@ -691,9 +684,7 @@ virNetDevVPortProfileOp8021Qbg(const char *ifname,
                                        vf,
                                        op,
                                        setlink_only);
-
-err_exit:
-
+cleanup:
     return rc;
 }