Update NetworkManager 0.9 connection states

RESOLVED FIXED in mozilla10

Status

()

Core
Networking
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Dan Williams, Assigned: nwgh)

Tracking

Trunk
mozilla10
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
Created attachment 517836 [details] [diff] [review]
Patch to update for NM 0.9 connection states.  Not compile tested...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → hurley
Depends on: 627672
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.)
Attachment #517836 - Flags: review?(bzbarsky)

Comment 3

6 years ago
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....
Attachment #517836 - Flags: review?(bzbarsky) → review+
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.
Attachment #517836 - Attachment is obsolete: true
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+
Attachment #528873 - Attachment is obsolete: true
Attachment #530327 - Flags: review+

Comment 6

6 years ago
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+
Attachment #530327 - Attachment is obsolete: true
Attachment #563374 - Flags: review+

Comment 7

6 years ago
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

6 years ago
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.
Attachment #563374 - Flags: checkin?

Updated

6 years ago
Keywords: checkin-needed
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).
Removing checkin-needed until comment 9 resolved.
Keywords: checkin-needed

Comment 11

6 years ago
(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.
(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.
(Reporter)

Comment 13

6 years ago
(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.
(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.

Updated

6 years ago
Attachment #563374 - Attachment is obsolete: true
Attachment #563374 - Flags: checkin?

Updated

6 years ago
Summary: [PATCH] update for NetworkManager 0.9 connection states → Update NetworkManager 0.9 connection states

Comment 15

6 years ago
Fix for bug 627672 has been checked in...can we proceed with this one?
Cleaning up the patch now (no longer applies to m-c), will push to try after that.

Comment 17

6 years ago
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
Attachment #563375 - Attachment is obsolete: true
Attachment #568093 - Flags: review+

Comment 18

6 years ago
(In reply to Takanori MATSUURA from comment #17)
> Carry over r=hurley
r=bzbarsky
Created attachment 568096 [details] [diff] [review]
Updated for bitrot + fix from last patch to carry forward r=bz
Attachment #568093 - Attachment is obsolete: true
Attachment #568096 - Flags: review+
Try is closed, will have to wait for that to clear up before pushing there (and then, presumably, to inbound)
https://tbpl.mozilla.org/?tree=Try&rev=6b795c09648c
Try looks good
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/924091e5589e
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla10
Version: unspecified → Trunk

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/924091e5589e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.