Re: [PATCH] nm-device: Remove bogus warning
Dear Colin, thank you for the patch! Am Dienstag, den 18.06.2013, 16:47 -0400 schrieb Colin Walters: This code was broken when it landed with commit fcc441622ae2632b9b36f352621cfd3baf34dc85 ; it was then later updated with 2318b3c5252403a52973eae70c32ff715c7994e7 Hmm, I am not good at memorizing commit hashes. In some projects the commit summary is also pasted and only the first 8 characters of the commit hash put in parentheses. But the assertion is just clearly wrong - if we're going to clear the previously queued state transition anyways, let's stop warning about it. Good find. The comment suggests though that this is not the intended behavior, right? So some kind of warning is warranted? With what device do you get the warning? I have not seen it. --- src/devices/nm-device.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) Is there a reason, why you attach the patch. Using Evolution, I always paste the whole content from the `git format-patch …` output into the message. Then I need to change the formatting from normal to pre-formatted to avoid automatic line breaks and it works. Putting it in the message makes replying and interleaved commenting easier. So just a suggestion. Thanks, Paul signature.asc Description: This is a digitally signed message part ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] nm-device: Remove bogus warning
On Tue, 2013-06-18 at 16:47 -0400, Colin Walters wrote: This code was broken when it landed with commit fcc441622ae2632b9b36f352621cfd3baf34dc85 ; it was then later updated with 2318b3c5252403a52973eae70c32ff715c7994e7 But the assertion is just clearly wrong - if we're going to clear the previously queued state transition anyways, let's stop warning about it. It's more there because it's potentially a bug if something double-queues, and I'd like to see that indication so we know that it happens, otherwise it's harder to track down. Any instance of this is technically a bug. What are you seeing it for? Dan ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] nm-device: Remove bogus warning
On Wed, 2013-06-19 at 08:11 -0500, Dan Williams wrote:
It's more there because it's potentially a bug
Ok, the logic seems odd because we're doing effectively:
if (condition)
{
if (condition)
warning ();
deal_with_it ();
}
If the code had looked like:
if (condition)
{
g_warn_if_reached ();
deal_with_it ();
}
it would have made more sense to me.
What are you seeing it for?
It always fires for me starting NM git master in my Fedora 19 test VM;
it doesn't for you?
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] nm-device: Remove bogus warning
Ok, how about this patch?
From ab22ffd804ba7e91bd6d064cbf5627ab03981c3b Mon Sep 17 00:00:00 2001
From: Colin Walters walt...@verbum.org
Date: Wed, 19 Jun 2013 14:56:26 -0400
Subject: [PATCH] device: Queuing two transitions to the same state is not an error
Just ignore this, since it happens in the current code and is
harmless. While we're here, improve the warning in the case where it
does occur to say whiich state we're overwriting. This should help
debug any future cases.
---
src/devices/nm-device.c | 11 ---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 0d741c6..dab951c 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -5587,7 +5587,12 @@ nm_device_queue_state (NMDevice *self,
/* We should only ever have one delayed state transition at a time */
if (priv-queued_state.id) {
- g_warn_if_fail (priv-queued_state.id == 0);
+ if (priv-queued_state.state == state)
+ return;
+ nm_log_warn (LOGD_DEVICE, (%s): overwriting previously queued state change to %s (%s),
+ nm_device_get_iface (self),
+ state_to_string (priv-queued_state.state),
+ reason_to_string (priv-queued_state.reason));
nm_device_queued_state_clear (self);
}
@@ -5595,8 +5600,8 @@ nm_device_queue_state (NMDevice *self,
priv-queued_state.reason = reason;
priv-queued_state.id = g_idle_add (queued_set_state, self);
- nm_log_dbg (LOGD_DEVICE, (%s): queued state change to %s (id %d),
- nm_device_get_iface (self), state_to_string (state),
+ nm_log_dbg (LOGD_DEVICE, (%s): queued state change to %s due to %s (id %d),
+ nm_device_get_iface (self), state_to_string (state), reason_to_string (reason),
priv-queued_state.id);
}
--
1.7.1
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] nm-device: Remove bogus warning
On Wed, 2013-06-19 at 16:01 -0400, Colin Walters wrote: Ok, how about this patch? Yeah, that's better. Dan ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
Re: [PATCH] nm-device: Remove bogus warning
On Wed, 2013-06-19 at 18:05 -0500, Dan Williams wrote: On Wed, 2013-06-19 at 16:01 -0400, Colin Walters wrote: Ok, how about this patch? Yeah, that's better. Pushed, thanks. Dan ___ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list
[PATCH] nm-device: Remove bogus warning
This code was broken when it landed with commit
fcc441622ae2632b9b36f352621cfd3baf34dc85 ; it was then later
updated with 2318b3c5252403a52973eae70c32ff715c7994e7
But the assertion is just clearly wrong - if we're going to clear the
previously queued state transition anyways, let's stop warning about
it.
---
src/devices/nm-device.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
From b485ce2c58e7d474392e8987fb834762beecc3ed Mon Sep 17 00:00:00 2001
From: Colin Walters walt...@verbum.org
Date: Tue, 18 Jun 2013 16:44:18 -0400
Subject: [PATCH] nm-device: Remove bogus warning
This code was broken when it landed with commit
fcc441622ae2632b9b36f352621cfd3baf34dc85 ; it was then later
updated with 2318b3c5252403a52973eae70c32ff715c7994e7
But the assertion is just clearly wrong - if we're going to clear the
previously queued state transition anyways, let's stop warning about
it.
---
src/devices/nm-device.c |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 0d741c6..dedc1e4 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -5587,7 +5587,6 @@ nm_device_queue_state (NMDevice *self,
/* We should only ever have one delayed state transition at a time */
if (priv-queued_state.id) {
- g_warn_if_fail (priv-queued_state.id == 0);
nm_device_queued_state_clear (self);
}
--
1.7.1
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list
