On 02/25/2015 11:37 AM, Michal Privoznik wrote:
On 25.02.2015 17:29, Laine Stump 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
On 02/25/2015 06:12 AM, Michal Privoznik wrote: 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.