Closed Bug 639959 Opened 14 years ago Closed 13 years ago

Update NetworkManager 0.9 connection states

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla10

People

(Reporter: dcbw, Assigned: u408661)

References

Details

Attachments

(1 file, 6 obsolete files)

Updated for bitrot + fix from last patch to carry forward r=bz
2.68 KB, patch
u408661
: review+
Details | Diff | Splinter Review
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
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 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+
Attached patch Updated patch for NM 0.9 (obsolete) — — Splinter Review
Fixed with bz's changes, dunno how to carry through r+, but it fits.
Attachment #517836 - Attachment is obsolete: true
Fixing up HG metadata for proper attribution. Carry forward r+
Attachment #528873 - Attachment is obsolete: true
Attachment #530327 - Flags: review+
Fit to the latest m-c. Carry forward r+
Attachment #530327 - Attachment is obsolete: true
Attachment #563374 - Flags: review+
Fix only the coding style to the latest one. No string changed.
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?
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
(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.
(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.
Attachment #563374 - Attachment is obsolete: true
Attachment #563374 - Flags: checkin?
Summary: [PATCH] update for NetworkManager 0.9 connection states → Update NetworkManager 0.9 connection states
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.
Fit to the latest m-c. Carry over r=hurley
Attachment #563375 - Attachment is obsolete: true
Attachment #568093 - Flags: review+
(In reply to Takanori MATSUURA from comment #17) > Carry over r=hurley r=bzbarsky
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)
Try looks good
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/924091e5589e
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla10
Version: unspecified → Trunk
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/924091e5589e
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: