1:39 p.m.
When John Ferlan ran the Coverity static checker on my patches to fix
the problem with nicindexes, it began complaining about calling the
virNetDevBandwidth* functions with a null ifname. Investigation
pointed out some real problems with this that are fixed by these two
patches (which will be pushed *before* the nicindex fix patch).
Laine Stump (2):
network: only clear bandwidth if it has been set
util: check for null ifname inside virNetDevBandwidthSet()
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 +++-
src/util/virnetdevbandwidth.c | 9 ++++++++-
src/util/virnetdevbandwidth.h | 4 ++--
6 files changed, 27 insertions(+), 9 deletions(-)
--
2.1.0
1:39 p.m.
New subject: [libvirt] [PATCH 1/2] network: only clear bandwidth if it has been set
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(-)
diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c
index c3e0f9f..42d42d6 100644
--- a/src/conf/netdev_bandwidth_conf.c
+++ b/src/conf/netdev_bandwidth_conf.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2009-2014 Red Hat, Inc.
+ * Copyright (C) 2009-2015 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
@@ -285,7 +285,9 @@ virDomainClearNetBandwidth(virDomainObjPtr vm)
for (i = 0; i < vm->def->nnets; i++) {
type = virDomainNetGetActualType(vm->def->nets[i]);
- if (virNetDevSupportBandwidth(type))
+ if (vm->def->nets[i]->ifname &&
+ virDomainNetGetActualBandwidth(vm->def->nets[i]) &&
+ virNetDevSupportBandwidth(type))
virNetDevBandwidthClear(vm->def->nets[i]->ifname);
}
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 3adb21d..87fcd98 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4650,13 +4650,18 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
actualType = virDomainNetGetActualType(detach);
/* clear network bandwidth */
- if (virNetDevSupportBandwidth(actualType) &&
+ if (detach->ifname &&
+ virDomainNetGetActualBandwidth(detach) &&
+ virNetDevSupportBandwidth(actualType) &&
virNetDevBandwidthClear(detach->ifname))
goto cleanup;
switch (actualType) {
case VIR_DOMAIN_NET_TYPE_BRIDGE:
case VIR_DOMAIN_NET_TYPE_NETWORK:
+ /* make sure Coverity knows that detach->ifname is valid */
+ sa_assert(detach->ifname);
+
if (virNetDevVethDelete(detach->ifname) < 0) {
virDomainAuditNet(vm, detach, NULL, "detach", false);
goto cleanup;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f240d3b..1209609 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2096,7 +2096,8 @@ networkStartNetworkVirtual(virNetworkObjPtr network)
return 0;
err5:
- virNetDevBandwidthClear(network->def->bridge);
+ if (network->def->bandwidth)
+ virNetDevBandwidthClear(network->def->bridge);
err4:
if (!save_err)
@@ -2142,7 +2143,8 @@ networkStartNetworkVirtual(virNetworkObjPtr network)
static int networkShutdownNetworkVirtual(virNetworkObjPtr network)
{
- virNetDevBandwidthClear(network->def->bridge);
+ if (network->def->bandwidth)
+ virNetDevBandwidthClear(network->def->bridge);
if (network->radvdPid > 0) {
char *radvdpidbase;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 8691c7e..9518abd 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3750,7 +3750,9 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
goto cleanup;
}
- if (virNetDevSupportBandwidth(virDomainNetGetActualType(detach)) &&
+ if (detach->ifname &&
+ virDomainNetGetActualBandwidth(detach) &&
+ virNetDevSupportBandwidth(virDomainNetGetActualType(detach)) &&
virNetDevBandwidthClear(detach->ifname) < 0)
VIR_WARN("cannot clear bandwidth setting for device : %s",
detach->ifname);
--
2.1.0
5:12 a.m.
New subject: [libvirt] [PATCH 1/2] network: only clear bandwidth if it has been set
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.
Michal
10:29 a.m.
New subject: [libvirt] [PATCH 1/2] network: only clear bandwidth if it has been set
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.
10:37 a.m.
New subject: [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.
Michal
12:08 p.m.
New subject: [libvirt] [PATCH 1/2] network: only clear bandwidth if it has been set
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.
1:39 p.m.
New subject: [libvirt] [PATCH 2/2] util: check for null ifname inside virNetDevBandwidthSet()
Previously this function relied on having ATTRIBUTE_NONNULL(1) in its
prototype rather than explicitly checking for a null
ifname. Unfortunately, ATTRIBUTE_NONNULL is just a hint to the
optimizer and code analyzers like Coverity, it doesn't actually check
anything at execution time, so the result was possible warnings from
Coverity, along with the possibility of null dereferences when ifname
wasn't available.
This patch removes the ATTRIBUTE_NONNULL from the prototype, and
checks ifname inside the function, logging an error if it's NULL (once
we've determined that the user really is trying to set a bandwidth).
---
src/util/virnetdevbandwidth.c | 9 ++++++++-
src/util/virnetdevbandwidth.h | 4 ++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index fbd2a8d..5de84a9 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2009-2014 Red Hat, Inc.
+ * Copyright (C) 2009-2015 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
@@ -83,6 +83,13 @@ virNetDevBandwidthSet(const char *ifname,
return -1;
}
+ if (!ifname) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Unable to set bandwidth for interface because "
+ "device name is unknown"));
+ return -1;
+ }
+
virNetDevBandwidthClear(ifname);
if (bandwidth->in && bandwidth->in->average) {
diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
index 4606a07..9b8f514 100644
--- a/src/util/virnetdevbandwidth.h
+++ b/src/util/virnetdevbandwidth.h
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2009-2013 Red Hat, Inc.
+ * Copyright (C) 2009-2015 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
@@ -46,7 +46,7 @@ void virNetDevBandwidthFree(virNetDevBandwidthPtr def);
int virNetDevBandwidthSet(const char *ifname,
virNetDevBandwidthPtr bandwidth,
bool hierarchical_class)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+ ATTRIBUTE_RETURN_CHECK;
int virNetDevBandwidthClear(const char *ifname)
ATTRIBUTE_NONNULL(1);
int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest,
--
2.1.0
3:40 a.m.
On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
When John Ferlan ran the Coverity static checker on my patches to fix
the problem with nicindexes, it began complaining about calling the
virNetDevBandwidth* functions with a null ifname. Investigation
pointed out some real problems with this that are fixed by these two
patches (which will be pushed *before* the nicindex fix patch).
Makes sense and works for me, ACK series after release.
Laine Stump (2):
network: only clear bandwidth if it has been set
util: check for null ifname inside virNetDevBandwidthSet()
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 +++-
src/util/virnetdevbandwidth.c | 9 ++++++++-
src/util/virnetdevbandwidth.h | 4 ++--
6 files changed, 27 insertions(+), 9 deletions(-)
--
2.1.0
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:30 a.m.
On 02/25/2015 04:40 AM, Martin Kletzander wrote:
On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
> When John Ferlan ran the Coverity static checker on my patches to fix
> the problem with nicindexes, it began complaining about calling the
> virNetDevBandwidth* functions with a null ifname. Investigation
> pointed out some real problems with this that are fixed by these two
> patches (which will be pushed *before* the nicindex fix patch).
>
Makes sense and works for me, ACK series after release.
No, these need to go in *before* the release, as does the patch for
nicindexes - otherwise macvtap and hostdev interfaces will not work.
10:57 a.m.
On 02/25/2015 11:30 AM, Laine Stump wrote:
On 02/25/2015 04:40 AM, Martin Kletzander wrote:
> On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
>> When John Ferlan ran the Coverity static checker on my patches to fix
>> the problem with nicindexes, it began complaining about calling the
>> virNetDevBandwidth* functions with a null ifname. Investigation
>> pointed out some real problems with this that are fixed by these two
>> patches (which will be pushed *before* the nicindex fix patch).
>>
>
> Makes sense and works for me, ACK series after release.
No, these need to go in *before* the release, as does the patch for
nicindexes - otherwise macvtap and hostdev interfaces will not work.
I agree - this release... For "reference" you have to see Laine's other
patch series which references commit id 'f7afeddc' which was a change
made during this release cycle...
John
12:31 a.m.
On Wed, Feb 25, 2015 at 11:57:47AM -0500, John Ferlan wrote:
On 02/25/2015 11:30 AM, Laine Stump wrote:
> On 02/25/2015 04:40 AM, Martin Kletzander wrote:
>> On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
>>> When John Ferlan ran the Coverity static checker on my patches to fix
>>> the problem with nicindexes, it began complaining about calling the
>>> virNetDevBandwidth* functions with a null ifname. Investigation
>>> pointed out some real problems with this that are fixed by these two
>>> patches (which will be pushed *before* the nicindex fix patch).
>>>
>>
>> Makes sense and works for me, ACK series after release.
>
> No, these need to go in *before* the release, as does the patch for
> nicindexes - otherwise macvtap and hostdev interfaces will not work.
>
I agree - this release... For "reference" you have to see Laine's other
patch series which references commit id 'f7afeddc' which was a change
made during this release cycle...
I was under the impression that the series Laine referred to has not
gone in and I jumped on this one since it can be reviewed without that
series I couldn't find. Anyway, sorry for my mistake then and if John
agree, then I see no point in postponing it.
3558
days inactive
3560
days old
10 comments
4 participants
participants (4)
-
John Ferlan -
Laine Stump -
Martin Kletzander -
Michal Privoznik