1:30 a.m.
Ján Tomko (2):
netdev: accept NULL in virNetDevSetupControl
bridge: don't crash on bandwidth unplug with no bandwidth
src/network/bridge_driver.c | 5 +++++
src/util/virnetdev.c | 14 ++++++++------
2 files changed, 13 insertions(+), 6 deletions(-)
--
1.8.1.5
1:30 a.m.
New subject: [libvirt] [PATCH 1/2] netdev: accept NULL in virNetDevSetupControl
Commit b9c6b073 dropped the version of virNetDevSetupControl
that didn't check for NULL arguments, but we call it like that
in virNetDevBridgeDelete.
---
src/util/virnetdev.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 7aba515..ebe20d0 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -62,13 +62,15 @@ static int virNetDevSetupControlFull(const char *ifname,
{
int fd;
- memset(ifr, 0, sizeof(*ifr));
+ if (ifr && ifname) {
+ memset(ifr, 0, sizeof(*ifr));
- if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) {
- virReportSystemError(ERANGE,
- _("Network interface name '%s' is too
long"),
- ifname);
- return -1;
+ if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) {
+ virReportSystemError(ERANGE,
+ _("Network interface name '%s' is too
long"),
+ ifname);
+ return -1;
+ }
}
if ((fd = socket(domain, type, 0)) < 0) {
--
1.8.1.5
1:45 a.m.
New subject: [libvirt] [PATCH 1/2] netdev: accept NULL in virNetDevSetupControl
On 06/21/2013 07:30 PM, Ján Tomko wrote:
Commit b9c6b073 dropped the version of virNetDevSetupControl
that didn't check for NULL arguments, but we call it like that
in virNetDevBridgeDelete.
---
src/util/virnetdev.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 7aba515..ebe20d0 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -62,13 +62,15 @@ static int virNetDevSetupControlFull(const char *ifname,
{
int fd;
- memset(ifr, 0, sizeof(*ifr));
+ if (ifr && ifname) {
+ memset(ifr, 0, sizeof(*ifr));
- if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) {
- virReportSystemError(ERANGE,
- _("Network interface name '%s' is too
long"),
- ifname);
- return -1;
+ if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) {
+ virReportSystemError(ERANGE,
+ _("Network interface name '%s' is too
long"),
+ ifname);
+ return -1;
+ }
}
if ((fd = socket(domain, type, 0)) < 0) {
I missed this:
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 933a9b3..44a37ca 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -38,7 +38,7 @@ typedef void virIfreq;
int virNetDevSetupControl(const char *ifname,
virIfreq *ifr)
- ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+ ATTRIBUTE_RETURN_CHECK;
int virNetDevExists(const char *brname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
Jan
1:57 a.m.
New subject: [libvirt] [PATCH 1/2] netdev: accept NULL in virNetDevSetupControl
On 06/21/2013 01:45 PM, Ján Tomko wrote:
On 06/21/2013 07:30 PM, Ján Tomko wrote:
> Commit b9c6b073 dropped the version of virNetDevSetupControl
> that didn't check for NULL arguments, but we call it like that
> in virNetDevBridgeDelete.
> ---
> src/util/virnetdev.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
ACK w/ the squashed change to virnetdev.h.
John
2:23 a.m.
New subject: [libvirt] [PATCH 1/2] netdev: accept NULL in virNetDevSetupControl
On 06/21/2013 07:57 PM, John Ferlan wrote:
On 06/21/2013 01:45 PM, Ján Tomko wrote:
> On 06/21/2013 07:30 PM, Ján Tomko wrote:
>> Commit b9c6b073 dropped the version of virNetDevSetupControl
>> that didn't check for NULL arguments, but we call it like that
>> in virNetDevBridgeDelete.
>> ---
>> src/util/virnetdev.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
ACK w/ the squashed change to virnetdev.h.
Thanks, pushed now.
Jan
1:30 a.m.
New subject: [libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth
If networkUnplugBandwidth is called on a network which has
no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if
bandwidth was removed from the network after the domain was
started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359
---
src/network/bridge_driver.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f7c2470..72a3f70 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual &&
iface->data.network.actual->class_id) {
+ if (!net->def->bandwidth || !net->def->bandwidth->in) {
+ VIR_WARN("Network %s has no bandwidth but unplug requested",
+ net->def->name);
+ goto cleanup;
+ }
/* we must remove class from bridge */
new_rate = net->def->bandwidth->in->average;
--
1.8.1.5
3:23 a.m.
New subject: [libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth
On 06/21/2013 01:30 PM, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has
no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if
s/bandwith/bandwidth
bandwidth was removed from the network after the domain was
started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359
---
src/network/bridge_driver.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f7c2470..72a3f70 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual &&
iface->data.network.actual->class_id) {
+ if (!net->def->bandwidth || !net->def->bandwidth->in) {
+ VIR_WARN("Network %s has no bandwidth but unplug requested",
+ net->def->name);
+ goto cleanup;
+ }
/* we must remove class from bridge */
new_rate = net->def->bandwidth->in->average;
I don't know the code well enough - I can understand why there's a crash
if bandwidth == NULL; however, it wasn't clear to me whether or not it
might be necessary to make the other calls that didn't rely on bandwidth
!= NULL but do pass/use class_id.
John
6:21 p.m.
New subject: [libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth
On 06/21/2013 09:23 PM, John Ferlan wrote:
On 06/21/2013 01:30 PM, Ján Tomko wrote:
> If networkUnplugBandwidth is called on a network which has
> no bandwidth defined, print a warning instead of crashing.
>
> This can happen when destroying a domain with bandwith if
s/bandwith/bandwidth
> bandwidth was removed from the network after the domain was
> started.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=975359
> ---
> src/network/bridge_driver.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index f7c2470..72a3f70 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
>
> if (iface->data.network.actual &&
> iface->data.network.actual->class_id) {
> + if (!net->def->bandwidth || !net->def->bandwidth->in) {
> + VIR_WARN("Network %s has no bandwidth but unplug requested",
> + net->def->name);
> + goto cleanup;
> + }
> /* we must remove class from bridge */
> new_rate = net->def->bandwidth->in->average;
>
>
I don't know the code well enough - I can understand why there's a crash
if bandwidth == NULL; however, it wasn't clear to me whether or not it
might be necessary to make the other calls that didn't rely on bandwidth
!= NULL but do pass/use class_id.
John
If I understand it correctly: if the network has no bandwidth, its bridge
shouldn't have any class_ids set and there should be nothing to clean up.
Jan
3:54 p.m.
New subject: [libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth
On 21.06.2013 19:30, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has
no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if
bandwidth was removed from the network after the domain was
started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359
---
src/network/bridge_driver.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f7c2470..72a3f70 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual &&
iface->data.network.actual->class_id) {
+ if (!net->def->bandwidth || !net->def->bandwidth->in) {
+ VIR_WARN("Network %s has no bandwidth but unplug requested",
+ net->def->name);
+ goto cleanup;
+ }
/* we must remove class from bridge */
new_rate = net->def->bandwidth->in->average;
So the problem is, if user starts a domain with interface which has
@floor set. The @floor requires a bandwidth to be set on the network the
interface is to be plugged into. Then he destroys the network, clear
<bandwidth/> from the network definition and starts the network again. I
think this is the place which should be fixed. We should deny removing
<bandwidth/> in case there's at least one interface attached with
@floor. Similarly, we refuse to start domain if the corresponding
network doesn't have any <bandwidth/> configured.
Michal
5:56 p.m.
New subject: [libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth
On 06/27/2013 09:54 AM, Michal Privoznik wrote:
On 21.06.2013 19:30, Ján Tomko wrote:
> If networkUnplugBandwidth is called on a network which has
> no bandwidth defined, print a warning instead of crashing.
>
> This can happen when destroying a domain with bandwith if
> bandwidth was removed from the network after the domain was
> started.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=975359
> ---
> src/network/bridge_driver.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index f7c2470..72a3f70 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
>
> if (iface->data.network.actual &&
> iface->data.network.actual->class_id) {
> + if (!net->def->bandwidth || !net->def->bandwidth->in) {
> + VIR_WARN("Network %s has no bandwidth but unplug requested",
> + net->def->name);
> + goto cleanup;
> + }
> /* we must remove class from bridge */
> new_rate = net->def->bandwidth->in->average;
>
>
So the problem is, if user starts a domain with interface which has
@floor set. The @floor requires a bandwidth to be set on the network the
interface is to be plugged into. Then he destroys the network, clear
<bandwidth/> from the network definition and starts the network again. I
think this is the place which should be fixed. We should deny removing
<bandwidth/> in case there's at least one interface attached with
@floor. Similarly, we refuse to start domain if the corresponding
network doesn't have any <bandwidth/> configured.
We refuse to start domains when the network isn't active too, even if they
don't have bandwidth, yet we allow the networks to be shut down afterwards.
I don't think we want to forbid shutting networks with bandwidth down (which
essentially removes all the bandwidth from it).
Jan
6:11 p.m.
New subject: [libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth
On 27.06.2013 11:56, Ján Tomko wrote:
On 06/27/2013 09:54 AM, Michal Privoznik wrote:
> On 21.06.2013 19:30, Ján Tomko wrote:
>> If networkUnplugBandwidth is called on a network which has
>> no bandwidth defined, print a warning instead of crashing.
>>
>> This can happen when destroying a domain with bandwith if
>> bandwidth was removed from the network after the domain was
>> started.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=975359
>> ---
>> src/network/bridge_driver.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index f7c2470..72a3f70 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
>>
>> if (iface->data.network.actual &&
>> iface->data.network.actual->class_id) {
>> + if (!net->def->bandwidth || !net->def->bandwidth->in) {
>> + VIR_WARN("Network %s has no bandwidth but unplug
requested",
>> + net->def->name);
>> + goto cleanup;
>> + }
>> /* we must remove class from bridge */
>> new_rate = net->def->bandwidth->in->average;
>>
>>
>
> So the problem is, if user starts a domain with interface which has
> @floor set. The @floor requires a bandwidth to be set on the network the
> interface is to be plugged into. Then he destroys the network, clear
> <bandwidth/> from the network definition and starts the network again. I
> think this is the place which should be fixed. We should deny removing
> <bandwidth/> in case there's at least one interface attached with
> @floor. Similarly, we refuse to start domain if the corresponding
> network doesn't have any <bandwidth/> configured.
We refuse to start domains when the network isn't active too, even if they
don't have bandwidth, yet we allow the networks to be shut down afterwards.
I don't think we want to forbid shutting networks with bandwidth down (which
essentially removes all the bandwidth from it).
Jan
Right. Good point, if user wants to shoot himself in the foot, we
shouldn't disallow him to do that (there's just too much of cases which
we would have to check). Libvirt's not foolproof. What we should do - or
rather not do is SIGSEGV-ing if user pull the trigger.
ACK then
Michal
6:17 p.m.
New subject: [libvirt] [PATCH 2/2] bridge: don't crash on bandwidth unplug with no bandwidth
On 06/27/2013 12:11 PM, Michal Privoznik wrote:
On 27.06.2013 11:56, Ján Tomko wrote:
> On 06/27/2013 09:54 AM, Michal Privoznik wrote:
>> On 21.06.2013 19:30, Ján Tomko wrote:
>>> If networkUnplugBandwidth is called on a network which has
>>> no bandwidth defined, print a warning instead of crashing.
>>>
>>> This can happen when destroying a domain with bandwith if
>>> bandwidth was removed from the network after the domain was
>>> started.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=975359
>>> ---
>>> src/network/bridge_driver.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>
>> So the problem is, if user starts a domain with interface which has
>> @floor set. The @floor requires a bandwidth to be set on the network the
>> interface is to be plugged into. Then he destroys the network, clear
>> <bandwidth/> from the network definition and starts the network again. I
>> think this is the place which should be fixed. We should deny removing
>> <bandwidth/> in case there's at least one interface attached with
>> @floor. Similarly, we refuse to start domain if the corresponding
>> network doesn't have any <bandwidth/> configured.
>
> We refuse to start domains when the network isn't active too, even if they
> don't have bandwidth, yet we allow the networks to be shut down afterwards.
>
> I don't think we want to forbid shutting networks with bandwidth down (which
> essentially removes all the bandwidth from it).
>
> Jan
Right. Good point, if user wants to shoot himself in the foot, we
shouldn't disallow him to do that (there's just too much of cases which
we would have to check). Libvirt's not foolproof. What we should do - or
rather not do is SIGSEGV-ing if user pull the trigger.
ACK then
Thanks, pushed now.
Jan
4144
days inactive
4150
days old
11 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik