[libvirt] [PATCH] virNetDevBandwidthClear: Improve error handling

[Peter Krempa]

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