[libvirt] [PATCH 1/2] network: only clear bandwidth if it has been set

[Laine Stump]

On 02/25/2015 11:37 AM, Michal Privoznik wrote:
> On 25.02.2015 17:29, Laine Stump wrote:
>> On 02/25/2015 06:12 AM, Michal Privoznik wrote:
>>> On 24.02.2015 20:39, Laine Stump wrote:
>>>> libvirt was unconditionally calling virNetDevBandwidthClear() for
>>>> every interface (and network bridge) of a type that supported
>>>> bandwidth, whether it actually had anything set or not. This doesn't
>>>> hurt anything (unless ifname == NULL!), but is wasteful.
>>>>
>>>> This patch makes sure that all calls to virNetDevBandwidthClear() are
>>>> qualified by checking that ifname != NULL and that it really had some
>>>> bandwidth setup done.
>>>>
>>>> (NB: an sa_assert(detach->ifname) was added in
>>>> lxcDomainDetachDeviceNetLive() because the preceding new check of
>>>> detach->ifname before calling virNetDevBandwidthClear() caused
>>>> Coverity to complain about a possible null dereference.)
>>>> ---
>>>>  src/conf/netdev_bandwidth_conf.c | 6 ++++--
>>>>  src/lxc/lxc_driver.c             | 7 ++++++-
>>>>  src/network/bridge_driver.c      | 6 ++++--
>>>>  src/qemu/qemu_hotplug.c          | 4 +++-
>>>>  4 files changed, 17 insertions(+), 6 deletions(-)
>>>>
>>> How about moving the check for non-NULL ifname into
>>> virNetDevBandwidthClear()? Its counterpart virNetDevBandwidthSet() does
>>> something similar over @bandwidth argument. And after 2/2 it'll done the
>>> same over @ifname.
>> Yeah, I had some reason for doing it externally originally, but then
>> forgot the reason (or it went away), then after I fixed
>> virNetDevBandwidthSet() I thought about changing this one, and decided
>> against it, mostly because I was tired and couldn't be certain that I
>> *didn't* have a valid reason in the first place. I'll do another spin
>> that way.
>>
> I don't want to discourage you to post patches, but I believe they are
> easy enough to be fixed locally and pushed. I don't want to enforce v2
> just because of this small nit.

In the past I have done seemingly trivial things in last minute "minor"
tweaks to patches that ended up breaking the build, so I tend to err on
the side of caution (especially when nobody actually says "ACK with
these nits fixed" :-)). This time I have triple-checked that I did make
check and make syntax-check with the tweaked patches, and have even
built an rpm, installed it, and tested it just to make sure it's okay.
So combined with assuming that your above statement is an unspoken ACK,
(and because macvtap and hostdev are busted until this gets fixed) I'm
pushing the patches.

Thanks to everyone for the reviews and suggestions.