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

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.