3:24 a.m.
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.
---
src/util/virnetdev.c | 38 ++++------------
src/util/virnetdevmacvlan.c | 37 +++++-----------
src/util/virnetdevvportprofile.c | 87 +++++++++++++++++---------------------
3 files changed, 60 insertions(+), 102 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2cc699a..6eec5cf 100644
--- 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 647679f..d467990 100644
--- 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 bd356d8..5b562be 100644
--- 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;
}
--
1.7.7.6