639959 - Update NetworkManager 0.9 connection states

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dan Williams]

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]

Attachment: Patch to update for NM 0.9 connection states. Not compile tested...

Comment 2

[u408661]

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 [:bzbarsky]]

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

[u408661]

Attachment: Updated patch for NM 0.9

Fixed with bz's changes, dunno how to carry through r+, but it fits.

Comment 5

[u408661]

Attachment: Updated patch for NM 0.9 (with correct hg metadata)

Fixing up HG metadata for proper attribution. Carry forward r+

Comment 6

[Takanori MATSUURA]

Attachment: 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]

Attachment: fit coding style to the latest one

Fix only the coding style to the latest one.
No string changed.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

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

[u408661]

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]]

Removing checkin-needed until comment 9 resolved.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

(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

[u408661]

(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]

(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

[u408661]

(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ý [:stransky] (ni? me)]

Fix for bug 627672 has been checked in...can we proceed with this one?

Comment 16

[u408661]

Cleaning up the patch now (no longer applies to m-c), will push to try after that.

Comment 17

[Takanori MATSUURA]

Attachment: 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]

(In reply to Takanori MATSUURA from comment #17)
> Carry over r=hurley
r=bzbarsky

Comment 19

[u408661]

Attachment: Updated for bitrot + fix from last patch to carry forward r=bz

Comment 20

[u408661]

Try is closed, will have to wait for that to clear up before pushing there (and then, presumably, to inbound)

Comment 21

[u408661]

https://tbpl.mozilla.org/?tree=Try&rev=6b795c09648c

Comment 22

[u408661]

Try looks good

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/924091e5589e

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/924091e5589e