Last Comment Bug 639959 - Update NetworkManager 0.9 connection states
: Update NetworkManager 0.9 connection states
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla10
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
Mentors:
Depends on: 627672
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-08 12:36 PST by Dan Williams
Modified: 2011-10-22 06:06 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to update for NM 0.9 connection states. Not compile tested... (1.91 KB, patch)
2011-03-08 12:37 PST, Dan Williams
bzbarsky: review+
Details | Diff | Splinter Review
Updated patch for NM 0.9 (2.73 KB, patch)
2011-04-28 09:20 PDT, Nicholas Hurley [:nwgh][:hurley]
no flags Details | Diff | Splinter Review
Updated patch for NM 0.9 (with correct hg metadata) (2.73 KB, patch)
2011-05-05 08:47 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Splinter Review
Updated patch for NM 0.9 (with correct hg metadata) (fit to the latest m-c) (2.84 KB, patch)
2011-09-29 05:19 PDT, Takanori MATSUURA
t.matsuu: review+
Details | Diff | Splinter Review
fit coding style to the latest one (2.99 KB, patch)
2011-09-29 05:19 PDT, Takanori MATSUURA
no flags Details | Diff | Splinter Review
Updated patch for NM 0.9 (with correct hg metadata) (fit to the latest m-c) (2.81 KB, patch)
2011-10-19 09:45 PDT, Takanori MATSUURA
t.matsuu: review+
Details | Diff | Splinter Review
Updated for bitrot + fix from last patch to carry forward r=bz (2.68 KB, patch)
2011-10-19 09:50 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Splinter Review

Description Dan Williams 2011-03-08 12:36:06 PST
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Build Identifier: 

NM 0.9 adds a few new connection states.  Update the code for these states while preserving backwards compatibility with previous versions of NetworkManager.

Reproducible: Always
Comment 1 Dan Williams 2011-03-08 12:37:56 PST
Created attachment 517836 [details] [diff] [review]
Patch to update for NM 0.9 connection states.  Not compile tested...
Comment 2 Nicholas Hurley [:nwgh][:hurley] 2011-04-01 14:40:46 PDT
Comment on attachment 517836 [details] [diff] [review]
Patch to update for NM 0.9 connection states.  Not compile tested...

Patch compiles (and works) fine, as long as the patch for bug 627672 is also applied. (It compiles with or without the other patch, but it's kind of worthless without it.)
Comment 3 Boris Zbarsky [:bz] 2011-04-22 22:06:13 PDT
Comment on attachment 517836 [details] [diff] [review]
Patch to update for NM 0.9 connection states.  Not compile tested...

Please document that NM_STATE_CONNECTED_OLD is what old dbus versions send?  Possibly even with a number attached to "old" when describing them (like "before version NNN").

Also, remove the extraneous parens in the assignment to mLinkUp.

r=me with those changes.  I'm really sorry for the lag here....
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2011-04-28 09:20:19 PDT
Created attachment 528873 [details] [diff] [review]
Updated patch for NM 0.9

Fixed with bz's changes, dunno how to carry through r+, but it fits.
Comment 5 Nicholas Hurley [:nwgh][:hurley] 2011-05-05 08:47:08 PDT
Created attachment 530327 [details] [diff] [review]
Updated patch for NM 0.9 (with correct hg metadata)

Fixing up HG metadata for proper attribution. Carry forward r+
Comment 6 Takanori MATSUURA 2011-09-29 05:19:01 PDT
Created attachment 563374 [details] [diff] [review]
Updated patch for NM 0.9 (with correct hg metadata) (fit to the latest m-c)

Fit to the latest m-c. Carry forward r+
Comment 7 Takanori MATSUURA 2011-09-29 05:19:39 PDT
Created attachment 563375 [details] [diff] [review]
fit coding style to the latest one

Fix only the coding style to the latest one.
No string changed.
Comment 8 Martin Stránský 2011-10-13 07:49:23 PDT
Comment on attachment 563374 [details] [diff] [review]
Updated patch for NM 0.9 (with correct hg metadata) (fit to the latest m-c)

Can we get this patch checked-in? It's needed for Bug 627672.
Comment 9 Nicholas Hurley [:nwgh][:hurley] 2011-10-13 08:58:03 PDT
Martin - is there some code change in this that is actually needed for bug 627672? If there isn't, the dependency seems to be in the opposite direction, as this patch is non-functional without the changes in 627672 (as I found when I tested it long ago).
Comment 10 Ed Morley [:emorley] 2011-10-14 03:06:33 PDT
Removing checkin-needed until comment 9 resolved.
Comment 11 Martin Stránský 2011-10-14 03:15:46 PDT
(In reply to Nick Hurley from comment #9)
> Martin - is there some code change in this that is actually needed for bug
> 627672? If there isn't, the dependency seems to be in the opposite
> direction, as this patch is non-functional without the changes in 627672 (as
> I found when I tested it long ago).

The patch works as expected (I've just tested it on latest trunk, with a patch from 627672). The patch works w/o 627672 - it fixes the DBUS/NM interface for NM >= 0.9, no matter how it's used later.
Comment 12 Nicholas Hurley [:nwgh][:hurley] 2011-10-14 06:19:14 PDT
(In reply to Martin Stránský from comment #11)
> The patch works as expected (I've just tested it on latest trunk, with a
> patch from 627672). The patch works w/o 627672 - it fixes the DBUS/NM
> interface for NM >= 0.9, no matter how it's used later.

Right, I believe it compiles with or without the patch from 627672, but does the detection of link state (what this patch actually fixes) work without that patch? Last  time I tested, it didn't, which is why I marked this as depending on 627672. If things have changed (or the other patch breaks gecko without this one), then let's go ahead and get this landed, otherwise it doesn't seem to make sense to me to land this when it does nothing useful without the other patch.
Comment 13 Dan Williams 2011-10-14 07:28:44 PDT
(In reply to Nick Hurley from comment #12)
> (In reply to Martin Stránský from comment #11)
> > The patch works as expected (I've just tested it on latest trunk, with a
> > patch from 627672). The patch works w/o 627672 - it fixes the DBUS/NM
> > interface for NM >= 0.9, no matter how it's used later.
> 
> Right, I believe it compiles with or without the patch from 627672, but does
> the detection of link state (what this patch actually fixes) work without
> that patch? Last  time I tested, it didn't, which is why I marked this as
> depending on 627672. If things have changed (or the other patch breaks gecko
> without this one), then let's go ahead and get this landed, otherwise it
> doesn't seem to make sense to me to land this when it does nothing useful
> without the other patch.

I do not believe this patch depends on the one in 627672.  Obviously if bug 627672's patch is required to get D-Bus working in the first place, then this patch wont' have any effect when applied.  But there's nothing in this patch that would *regress* the current situation, and then when bug 627672 got fixed things would magically work.
Comment 14 Nicholas Hurley [:nwgh][:hurley] 2011-10-14 07:43:10 PDT
(In reply to Dan Williams from comment #13)
> I do not believe this patch depends on the one in 627672.  Obviously if bug
> 627672's patch is required to get D-Bus working in the first place, then
> this patch wont' have any effect when applied.  But there's nothing in this
> patch that would *regress* the current situation, and then when bug 627672
> got fixed things would magically work.

We're in agreement on everything except the dependency :) My view is that, since this patch is a no-op without bug 627672, we shouldn't land this until 627672 does, as the functionality is unverifiable without that other patch. I'm cc'ing Jason to get a necko peer's opinion on this matter. If he says land it, then let's land it and just tell QA to wait to verify until 627672 also lands.
Comment 15 Martin Stránský 2011-10-19 05:29:44 PDT
Fix for bug 627672 has been checked in...can we proceed with this one?
Comment 16 Nicholas Hurley [:nwgh][:hurley] 2011-10-19 08:47:25 PDT
Cleaning up the patch now (no longer applies to m-c), will push to try after that.
Comment 17 Takanori MATSUURA 2011-10-19 09:45:44 PDT
Created attachment 568093 [details] [diff] [review]
Updated patch for NM 0.9 (with correct hg metadata) (fit to the latest m-c)

Fit to the latest m-c.
Carry over r=hurley
Comment 18 Takanori MATSUURA 2011-10-19 09:48:11 PDT
(In reply to Takanori MATSUURA from comment #17)
> Carry over r=hurley
r=bzbarsky
Comment 19 Nicholas Hurley [:nwgh][:hurley] 2011-10-19 09:50:53 PDT
Created attachment 568096 [details] [diff] [review]
Updated for bitrot + fix from last patch to carry forward r=bz
Comment 20 Nicholas Hurley [:nwgh][:hurley] 2011-10-19 10:00:30 PDT
Try is closed, will have to wait for that to clear up before pushing there (and then, presumably, to inbound)
Comment 21 Nicholas Hurley [:nwgh][:hurley] 2011-10-19 15:23:58 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6b795c09648c
Comment 22 Nicholas Hurley [:nwgh][:hurley] 2011-10-20 09:21:08 PDT
Try looks good
Comment 24 Ed Morley [:emorley] 2011-10-22 06:06:52 PDT
https://hg.mozilla.org/mozilla-central/rev/924091e5589e

Note You need to log in before you can comment on or make changes to this bug.