[libvirt PATCH 0/6] util: virnetdev: use g_auto for virCommand
Refactor some functions to remove more manual virCommand freeing. virNetDevBandwidthSet is left untouched. Ján Tomko (6): util: midonet: use g_auto for virCommand util: virNetDevBandwidthUpdateRate: refactor util: virNetDevBandwidthManipulateFilter: use g_auto util: virNetDevBandwidthClear: use g_auto util: refactor virNetDevBandwidthPlug util: refactor virNetDevBandwidthUnplug src/util/virnetdevbandwidth.c | 128 +++++++++++++--------------------- src/util/virnetdevmidonet.c | 20 ++---- 2 files changed, 53 insertions(+), 95 deletions(-) -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnetdevmidonet.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/util/virnetdevmidonet.c b/src/util/virnetdevmidonet.c index 9061f1516f..72b2fd4467 100644 --- a/src/util/virnetdevmidonet.c +++ b/src/util/virnetdevmidonet.c @@ -39,8 +39,7 @@ int virNetDevMidonetBindPort(const char *ifname, const virNetDevVPortProfile *virtualport) { - int ret = -1; - virCommand *cmd = NULL; + g_autoptr(virCommand) cmd = NULL; char virtportuuid[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(virtualport->interfaceID, virtportuuid); @@ -53,13 +52,10 @@ virNetDevMidonetBindPort(const char *ifname, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to bind port %s to the virtual port %s"), ifname, virtportuuid); - goto cleanup; + return -1; } - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return 0; } /** @@ -73,8 +69,7 @@ virNetDevMidonetBindPort(const char *ifname, int virNetDevMidonetUnbindPort(const virNetDevVPortProfile *virtualport) { - int ret = -1; - virCommand *cmd = NULL; + g_autoptr(virCommand) cmd = NULL; char virtportuuid[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(virtualport->interfaceID, virtportuuid); @@ -86,11 +81,8 @@ virNetDevMidonetUnbindPort(const virNetDevVPortProfile *virtualport) virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to unbind the virtual port %s from Midonet"), virtportuuid); - goto cleanup; + return -1; } - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return 0; } -- 2.31.1
Use automatic cleanup and remove the 'ret' variable in favor of direct returns. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnetdevbandwidth.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 75fc5607ad..083b860059 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -685,11 +685,10 @@ virNetDevBandwidthUpdateRate(const char *ifname, virNetDevBandwidth *bandwidth, unsigned long long new_rate) { - int ret = -1; - virCommand *cmd = NULL; - char *class_id = NULL; - char *rate = NULL; - char *ceil = NULL; + g_autoptr(virCommand) cmd = NULL; + g_autofree char *class_id = NULL; + g_autofree char *rate = NULL; + g_autofree char *ceil = NULL; class_id = g_strdup_printf("1:%x", id); rate = g_strdup_printf("%llukbps", new_rate); @@ -703,17 +702,7 @@ virNetDevBandwidthUpdateRate(const char *ifname, "ceil", ceil, NULL); virNetDevBandwidthCmdAddOptimalQuantum(cmd, bandwidth->in); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virCommandFree(cmd); - VIR_FREE(class_id); - VIR_FREE(rate); - VIR_FREE(ceil); - return ret; + return virCommandRun(cmd, NULL); } /** -- 2.31.1
Reduce the scope of the variable to avoid renaming it. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnetdevbandwidth.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 083b860059..fe354696f2 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -105,8 +105,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname, bool create_new) { int ret = -1; - char *filter_id = NULL; - virCommand *cmd = NULL; + g_autofree char *filter_id = NULL; unsigned char ifmac[VIR_MAC_BUFLEN]; char *mac[2] = {NULL, NULL}; @@ -120,9 +119,9 @@ virNetDevBandwidthManipulateFilter(const char *ifname, filter_id = g_strdup_printf("800::%u", id); if (remove_old) { + g_autoptr(virCommand) cmd = virCommandNew(TC); int cmd_ret = 0; - cmd = virCommandNew(TC); virCommandAddArgList(cmd, "filter", "del", "dev", ifname, "prio", "2", "handle", filter_id, "u32", NULL); @@ -132,14 +131,13 @@ virNetDevBandwidthManipulateFilter(const char *ifname, } if (create_new) { + g_autoptr(virCommand) cmd = virCommandNew(TC); virMacAddrGetRaw(ifmac_ptr, ifmac); mac[0] = g_strdup_printf("0x%02x%02x%02x%02x", ifmac[2], ifmac[3], ifmac[4], ifmac[5]); mac[1] = g_strdup_printf("0x%02x%02x", ifmac[0], ifmac[1]); - virCommandFree(cmd); - cmd = virCommandNew(TC); /* Okay, this not nice. But since libvirt does not necessarily track * interface IP address(es), and tc fw filter simply refuse to use * ebtables marks, we need to use u32 selector to match MAC address. @@ -160,8 +158,6 @@ virNetDevBandwidthManipulateFilter(const char *ifname, cleanup: VIR_FREE(mac[1]); VIR_FREE(mac[0]); - VIR_FREE(filter_id); - virCommandFree(cmd); return ret; } -- 2.31.1
Separate the two uses of 'cmd' to avoid mixing manual and automatic cleanup. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnetdevbandwidth.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index fe354696f2..cb9a22a369 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -416,27 +416,24 @@ virNetDevBandwidthClear(const char *ifname) { int ret = 0; int dummy; /* for ignoring the exit status */ - virCommand *cmd = NULL; + g_autoptr(virCommand) rootcmd = NULL; + g_autoptr(virCommand) ingresscmd = NULL; if (!ifname) return 0; - cmd = virCommandNew(TC); - virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "root", NULL); + rootcmd = virCommandNew(TC); + virCommandAddArgList(rootcmd, "qdisc", "del", "dev", ifname, "root", NULL); - if (virCommandRun(cmd, &dummy) < 0) + if (virCommandRun(rootcmd, &dummy) < 0) ret = -1; - virCommandFree(cmd); + ingresscmd = virCommandNew(TC); + virCommandAddArgList(ingresscmd, "qdisc", "del", "dev", ifname, "ingress", NULL); - cmd = virCommandNew(TC); - virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "ingress", NULL); - - if (virCommandRun(cmd, &dummy) < 0) + if (virCommandRun(ingresscmd, &dummy) < 0) ret = -1; - virCommandFree(cmd); - return ret; } -- 2.31.1
Use g_auto, split the double use of 'cmd' variable and remove useless ret variable. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnetdevbandwidth.c | 43 ++++++++++++++--------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index cb9a22a369..7f394926ef 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -538,12 +538,12 @@ virNetDevBandwidthPlug(const char *brname, virNetDevBandwidth *bandwidth, unsigned int id) { - int ret = -1; - virCommand *cmd = NULL; - char *class_id = NULL; - char *qdisc_id = NULL; - char *floor = NULL; - char *ceil = NULL; + g_autoptr(virCommand) cmd1 = NULL; + g_autoptr(virCommand) cmd2 = NULL; + g_autofree char *class_id = NULL; + g_autofree char *qdisc_id = NULL; + g_autofree char *floor = NULL; + g_autofree char *ceil = NULL; char ifmacStr[VIR_MAC_STRING_BUFLEN]; if (id <= 2) { @@ -568,37 +568,28 @@ virNetDevBandwidthPlug(const char *brname, net_bandwidth->in->peak : net_bandwidth->in->average); - cmd = virCommandNew(TC); - virCommandAddArgList(cmd, "class", "add", "dev", brname, "parent", "1:1", + cmd1 = virCommandNew(TC); + virCommandAddArgList(cmd1, "class", "add", "dev", brname, "parent", "1:1", "classid", class_id, "htb", "rate", floor, "ceil", ceil, NULL); - virNetDevBandwidthCmdAddOptimalQuantum(cmd, bandwidth->in); + virNetDevBandwidthCmdAddOptimalQuantum(cmd1, bandwidth->in); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + if (virCommandRun(cmd1, NULL) < 0) + return -1; - virCommandFree(cmd); - cmd = virCommandNew(TC); - virCommandAddArgList(cmd, "qdisc", "add", "dev", brname, "parent", + cmd2 = virCommandNew(TC); + virCommandAddArgList(cmd2, "qdisc", "add", "dev", brname, "parent", class_id, "handle", qdisc_id, "sfq", "perturb", "10", NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + if (virCommandRun(cmd2, NULL) < 0) + return -1; if (virNetDevBandwidthManipulateFilter(brname, ifmac_ptr, id, class_id, false, true) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - VIR_FREE(ceil); - VIR_FREE(floor); - VIR_FREE(qdisc_id); - VIR_FREE(class_id); - virCommandFree(cmd); - return ret; + return 0; } /* -- 2.31.1
Remove pointless 'ret', cmd variable reuse and use g_auto. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virnetdevbandwidth.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 7f394926ef..2895be8d27 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -605,11 +605,11 @@ int virNetDevBandwidthUnplug(const char *brname, unsigned int id) { - int ret = -1; int cmd_ret = 0; - virCommand *cmd = NULL; - char *class_id = NULL; - char *qdisc_id = NULL; + g_autoptr(virCommand) cmd1 = NULL; + g_autoptr(virCommand) cmd2 = NULL; + g_autofree char *class_id = NULL; + g_autofree char *qdisc_id = NULL; if (id <= 2) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid class ID %d"), id); @@ -619,34 +619,27 @@ virNetDevBandwidthUnplug(const char *brname, class_id = g_strdup_printf("1:%x", id); qdisc_id = g_strdup_printf("%x:", id); - cmd = virCommandNew(TC); - virCommandAddArgList(cmd, "qdisc", "del", "dev", brname, + cmd1 = virCommandNew(TC); + virCommandAddArgList(cmd1, "qdisc", "del", "dev", brname, "handle", qdisc_id, NULL); /* Don't threat tc errors as fatal, but * try to remove as much as possible */ - if (virCommandRun(cmd, &cmd_ret) < 0) - goto cleanup; + if (virCommandRun(cmd1, &cmd_ret) < 0) + return -1; if (virNetDevBandwidthManipulateFilter(brname, NULL, id, NULL, true, false) < 0) - goto cleanup; + return -1; - virCommandFree(cmd); - cmd = virCommandNew(TC); - virCommandAddArgList(cmd, "class", "del", "dev", brname, + cmd2 = virCommandNew(TC); + virCommandAddArgList(cmd2, "class", "del", "dev", brname, "classid", class_id, NULL); - if (virCommandRun(cmd, &cmd_ret) < 0) - goto cleanup; + if (virCommandRun(cmd2, &cmd_ret) < 0) + return -1; - ret = 0; - - cleanup: - VIR_FREE(qdisc_id); - VIR_FREE(class_id); - virCommandFree(cmd); - return ret; + return 0; } /** -- 2.31.1
On 1/17/22 18:02, Ján Tomko wrote:
Refactor some functions to remove more manual virCommand freeing.
virNetDevBandwidthSet is left untouched.
Ján Tomko (6): util: midonet: use g_auto for virCommand util: virNetDevBandwidthUpdateRate: refactor util: virNetDevBandwidthManipulateFilter: use g_auto util: virNetDevBandwidthClear: use g_auto util: refactor virNetDevBandwidthPlug util: refactor virNetDevBandwidthUnplug
src/util/virnetdevbandwidth.c | 128 +++++++++++++--------------------- src/util/virnetdevmidonet.c | 20 ++---- 2 files changed, 53 insertions(+), 95 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Ján Tomko -
Michal Prívozník