[libvirt] [PATCH] virNetDevBandwidthClear: Improve error handling
Two changes are introduced in this patch. The first change removes ATTRIBUTE_RETURN_CHECK from virNetDevBandwidthClear, because it was called with ignore_value always, anyway. The function is used even when it's not necessary to call it, just for cleanup purposes. The second change is an added parameter "ignore_error" for the same function, which basically means "Ignore any errors from the commands that will be run". This parameter, when true, doesn't suppress any libvirt errors, just the exit value of commands ran. --- src/network/bridge_driver.c | 4 ++-- src/util/virnetdevbandwidth.c | 15 ++++++++++----- src/util/virnetdevbandwidth.h | 6 +++--- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0e38016..f153cdd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2161,7 +2161,7 @@ networkStartNetworkVirtual(struct network_driver *driver, return 0; err5: - ignore_value(virNetDevBandwidthClear(network->def->bridge)); + virNetDevBandwidthClear(network->def->bridge, true); err4: if (!save_err) @@ -2206,7 +2206,7 @@ networkStartNetworkVirtual(struct network_driver *driver, static int networkShutdownNetworkVirtual(struct network_driver *driver, virNetworkObjPtr network) { - ignore_value(virNetDevBandwidthClear(network->def->bridge)); + virNetDevBandwidthClear(network->def->bridge, true); if (network->radvdPid > 0) { char *radvdpidbase; diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index b23ccd3..98f6b68 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -69,7 +69,7 @@ virNetDevBandwidthSet(const char *ifname, goto cleanup; } - ignore_value(virNetDevBandwidthClear(ifname)); + virNetDevBandwidthClear(ifname, true); if (bandwidth->in) { if (virAsprintf(&average, "%llukbps", bandwidth->in->average) < 0) @@ -163,15 +163,19 @@ cleanup: * Return 0 on success, -1 otherwise. */ int -virNetDevBandwidthClear(const char *ifname) +virNetDevBandwidthClear(const char *ifname, bool ignore_error) { int ret = 0; + int exitstatus, *es = NULL; virCommandPtr cmd = NULL; + if (ignore_error) + es = &exitstatus; + cmd = virCommandNew(TC); virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "root", NULL); - if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, es) < 0) ret = -1; virCommandFree(cmd); @@ -179,8 +183,9 @@ virNetDevBandwidthClear(const char *ifname) cmd = virCommandNew(TC); virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "ingress", NULL); - if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, es) < 0) ret = -1; + virCommandFree(cmd); return ret; diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 18384ae..f15b489 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -43,8 +43,8 @@ void virNetDevBandwidthFree(virNetDevBandwidthPtr def); int virNetDevBandwidthSet(const char *ifname, virNetDevBandwidthPtr bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int virNetDevBandwidthClear(const char *ifname) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virNetDevBandwidthClear(const char *ifname, bool ignore_error) + ATTRIBUTE_NONNULL(1); int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest, const virNetDevBandwidthPtr src) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -- 1.7.12
On 09/18/12 15:52, Martin Kletzander wrote:
Two changes are introduced in this patch. The first change removes ATTRIBUTE_RETURN_CHECK from virNetDevBandwidthClear, because it was called with ignore_value always, anyway. The function is used even when it's not necessary to call it, just for cleanup purposes. The second change is an added parameter "ignore_error" for the same function, which basically means "Ignore any errors from the commands that will be run". This parameter, when true, doesn't suppress any libvirt errors, just the exit value of commands ran. --- src/network/bridge_driver.c | 4 ++-- src/util/virnetdevbandwidth.c | 15 ++++++++++----- src/util/virnetdevbandwidth.h | 6 +++--- 3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0e38016..f153cdd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2161,7 +2161,7 @@ networkStartNetworkVirtual(struct network_driver *driver, return 0;
err5: - ignore_value(virNetDevBandwidthClear(network->def->bridge)); + virNetDevBandwidthClear(network->def->bridge, true);
err4: if (!save_err) @@ -2206,7 +2206,7 @@ networkStartNetworkVirtual(struct network_driver *driver, static int networkShutdownNetworkVirtual(struct network_driver *driver, virNetworkObjPtr network) { - ignore_value(virNetDevBandwidthClear(network->def->bridge)); + virNetDevBandwidthClear(network->def->bridge, true);
if (network->radvdPid > 0) { char *radvdpidbase; diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index b23ccd3..98f6b68 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -69,7 +69,7 @@ virNetDevBandwidthSet(const char *ifname, goto cleanup; }
- ignore_value(virNetDevBandwidthClear(ifname)); + virNetDevBandwidthClear(ifname, true);
All 3 of the call paths for this function call it with ignore_error set to true. I think that it's not necessary to have the feature to turn on errors now, if nobody actually uses it.
if (bandwidth->in) { if (virAsprintf(&average, "%llukbps", bandwidth->in->average) < 0) @@ -163,15 +163,19 @@ cleanup: * Return 0 on success, -1 otherwise. */ int -virNetDevBandwidthClear(const char *ifname) +virNetDevBandwidthClear(const char *ifname, bool ignore_error) { int ret = 0; + int exitstatus, *es = NULL; virCommandPtr cmd = NULL;
+ if (ignore_error) + es = &exitstatus; + cmd = virCommandNew(TC); virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "root", NULL);
- if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, es) < 0) ret = -1;
virCommandFree(cmd); @@ -179,8 +183,9 @@ virNetDevBandwidthClear(const char *ifname) cmd = virCommandNew(TC); virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "ingress", NULL);
- if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, es) < 0) ret = -1; + virCommandFree(cmd);
return ret; diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 18384ae..f15b489 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -43,8 +43,8 @@ void virNetDevBandwidthFree(virNetDevBandwidthPtr def);
int virNetDevBandwidthSet(const char *ifname, virNetDevBandwidthPtr bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int virNetDevBandwidthClear(const char *ifname) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virNetDevBandwidthClear(const char *ifname, bool ignore_error) + ATTRIBUTE_NONNULL(1); int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest, const virNetDevBandwidthPtr src) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
ACK if you leave out the ignore_error argument. Peter
On 09/18/2012 04:23 PM, Peter Krempa wrote:
On 09/18/12 15:52, Martin Kletzander wrote:
Two changes are introduced in this patch. The first change removes ATTRIBUTE_RETURN_CHECK from virNetDevBandwidthClear, because it was called with ignore_value always, anyway. The function is used even when it's not necessary to call it, just for cleanup purposes. The second change is an added parameter "ignore_error" for the same function, which basically means "Ignore any errors from the commands that will be run". This parameter, when true, doesn't suppress any libvirt errors, just the exit value of commands ran. --- src/network/bridge_driver.c | 4 ++-- src/util/virnetdevbandwidth.c | 15 ++++++++++----- src/util/virnetdevbandwidth.h | 6 +++--- 3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0e38016..f153cdd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2161,7 +2161,7 @@ networkStartNetworkVirtual(struct network_driver *driver, return 0;
err5: - ignore_value(virNetDevBandwidthClear(network->def->bridge)); + virNetDevBandwidthClear(network->def->bridge, true);
err4: if (!save_err) @@ -2206,7 +2206,7 @@ networkStartNetworkVirtual(struct network_driver *driver, static int networkShutdownNetworkVirtual(struct network_driver *driver, virNetworkObjPtr network) { - ignore_value(virNetDevBandwidthClear(network->def->bridge)); + virNetDevBandwidthClear(network->def->bridge, true);
if (network->radvdPid > 0) { char *radvdpidbase; diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index b23ccd3..98f6b68 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -69,7 +69,7 @@ virNetDevBandwidthSet(const char *ifname, goto cleanup; }
- ignore_value(virNetDevBandwidthClear(ifname)); + virNetDevBandwidthClear(ifname, true);
All 3 of the call paths for this function call it with ignore_error set to true. I think that it's not necessary to have the feature to turn on errors now, if nobody actually uses it.
if (bandwidth->in) { if (virAsprintf(&average, "%llukbps", bandwidth->in->average) < 0) @@ -163,15 +163,19 @@ cleanup: * Return 0 on success, -1 otherwise. */ int -virNetDevBandwidthClear(const char *ifname) +virNetDevBandwidthClear(const char *ifname, bool ignore_error) { int ret = 0; + int exitstatus, *es = NULL; virCommandPtr cmd = NULL;
+ if (ignore_error) + es = &exitstatus; + cmd = virCommandNew(TC); virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "root", NULL);
- if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, es) < 0) ret = -1;
virCommandFree(cmd); @@ -179,8 +183,9 @@ virNetDevBandwidthClear(const char *ifname) cmd = virCommandNew(TC); virCommandAddArgList(cmd, "qdisc", "del", "dev", ifname, "ingress", NULL);
- if (virCommandRun(cmd, NULL) < 0) + if (virCommandRun(cmd, es) < 0) ret = -1; + virCommandFree(cmd);
return ret; diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 18384ae..f15b489 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -43,8 +43,8 @@ void virNetDevBandwidthFree(virNetDevBandwidthPtr def);
int virNetDevBandwidthSet(const char *ifname, virNetDevBandwidthPtr bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int virNetDevBandwidthClear(const char *ifname) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virNetDevBandwidthClear(const char *ifname, bool ignore_error) + ATTRIBUTE_NONNULL(1); int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest, const virNetDevBandwidthPtr src) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
ACK if you leave out the ignore_error argument.
Peter
Fixed as you requested and pushed, thanks. Martin
participants (2)
-
Martin Kletzander -
Peter Krempa