[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virNetDevBandwidthClear: Improve error handling



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]