Tails

#1 Updated by anonym 2015-09-11 04:24:16

• Status changed from Confirmed to In Progress

Applied in changeset commit:95de82a5848f5a4c83632e85bc74f805d9bf9161.

#2 Updated by anonym 2015-09-11 04:26:04

• Assignee changed from anonym to intrigeri
• Target version set to Tails_1.6
• % Done changed from 0 to 50
• QA Check set to Ready for QA
• Feature Branch set to bugfix/7689-fix-tails-restricted-network-detector

> Use of uninitialized value $state{“eth1”} in string eq at
> /usr/local/sbin/tails-restricted-network-detector line 72.

Ah, that makes sense. If the case

/\(([^)]+)\): supplicant connection state:.*-> (.*)$/

doesn’t happen before NetworkManager detects a timeout with the case

/Activation \(([^)]+)\/[^)]*\): association took too long/

then $state{"eth1"} will be used while it is set to undef (from the first case). Instead of setting it to undef we should just set it to the empty string.

intrigeri, you being our resident perl expert, does this make sense to you?

#4 Updated by anonym 2015-09-14 06:06:49

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

#5 Updated by anonym 2015-09-14 07:02:21

• Assignee changed from anonym to intrigeri
• QA Check changed from Dev Needed to Ready for QA

See commit commit:579f2d8 for the other bugfix. Also see the automated test added in Feature #6302’s branch test/6302-mac-spoofing.

#7 Updated by intrigeri 2015-09-15 08:38:18

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Info Needed

> See commit commit:579f2d8 for the other bugfix. Also see the automated test added in Feature #6302’s branch test/6302-mac-spoofing.

I’m not sure I understand why this commit is needed, and its message doesn’t help me much.

My understanding is that we don’t really care about recording the exact state, we only care about whether it’s “associating” or not, right? So we need properly record state changes to/from “associating”. I guess that this commit fixes a case you’ve seen (“supplicant interface state: authenticating -> associating”) that was previously not supported (by the way, I wonder if that’s maybe broken since we moved to Wheezy?). So far, so good. So why are we recording the “device state change” as well? Just to make sure we record an accurate state when we leave “associating” state? If that’s what we want to do, then I think we may need to be more aggressive, ignore the first part of the message and match on all state change: \S+ -> (\S+)/. What do you think?

Perhaps all I need is a better commit message (and I see this branch was merged into the one for Feature #6302, enjoy ;)

#8 Updated by anonym 2015-09-16 12:11:55

• Assignee changed from anonym to intrigeri

intrigeri wrote:
> > See commit commit:579f2d8 for the other bugfix. Also see the automated test added in Feature #6302’s branch test/6302-mac-spoofing.
>
> I’m not sure I understand why this commit is needed, and its message doesn’t help me much.
>
> My understanding is that we don’t really care about recording the exact state, we only care about whether it’s “associating” or not, right? So we need properly record state changes to/from “associating”. I guess that this commit fixes a case you’ve seen (“supplicant interface state: authenticating -> associating”) that was previously not supported

Exactly.

> (by the way, I wonder if that’s maybe broken since we moved to Wheezy?).

Maybe. :) Unfortunately my automated test won’t necessarily solve this problem. :/

> So far, so good.

Then I think you are understanding me.

> So why are we recording the “device state change” as well? Just to make sure we record an accurate state when we leave “associating” state?

Yes.

> If that’s what we want to do, then I think we may need to be more aggressive, ignore the first part of the message and match on all state change: \S+ -> (\S+)/. What do you think?

Well, I haven’t checked whether that may catch something strange. Is this really necessary?

> Perhaps all I need is a better commit message (and I see this branch was merged into the one for Feature #6302, enjoy ;)

I guess this is clear now. Sorry for the bad commit message, but I won’t rewrite the history. If you like I can push a comment that explains it. What do you think?

#9 Updated by intrigeri 2015-09-16 12:52:33

• Assignee changed from intrigeri to anonym

> Well, I haven’t checked whether that may catch something strange. Is this really necessary?

If it doesn’t seem needed at first glance to you, forget it :)

>> Perhaps all I need is a better commit message (and I see this branch was merged into the one for Feature #6302, enjoy ;)

> I guess this is clear now. Sorry for the bad commit message, but I won’t rewrite the history. If you like I can push a comment that explains it. What do you think?

Yes, please :)

#10 Updated by anonym 2015-09-16 13:15:17

• Assignee changed from anonym to intrigeri
• QA Check changed from Info Needed to Ready for QA

#11 Updated by intrigeri 2015-09-16 13:52:30

• % Done changed from 50 to 60

Code review passes, building.

#12 Updated by intrigeri 2015-09-17 02:41:15

• Status changed from In Progress to Fix committed
• % Done changed from 60 to 100

Applied in changeset commit:97e28d442b028eeb988c1a1c367092fbaafabf55.

#13 Updated by intrigeri 2015-09-17 02:42:24

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Pass

I’m afraid I could not test it with a restricted network, but I trust anonym to have done it, and the code changes make sense to me.

#14 Updated by bertagaz 2015-09-22 15:10:20

• Status changed from Fix committed to Resolved
• QA Check deleted (Pass)