Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alfred: Support interface IDs with more than two digits #354

Merged
merged 1 commit into from May 2, 2018

Conversation

adschm
Copy link
Member

@adschm adschm commented Mar 20, 2018

Occationally /proc/net/if_inet6 contains interface IDs with
three digits. In this case, the regex in wait_for_ll_address()
does not work anymore and alfred is not starting.

@mwarning
Copy link
Contributor

mwarning commented Apr 6, 2018

@ecsv could you take a look at this PR?

@ecsv
Copy link
Contributor

ecsv commented Apr 6, 2018

I think you should ask @NeoRaider because he implemented the original code for this.

@ecsv
Copy link
Contributor

ecsv commented Apr 6, 2018

Hm, just had a look at the change and the commit message. The commit forgot to update the comment above the awk code. And the commit message doesn't explain the problem very well.

The current output of looks like this

root@PL-See68-DG:~# cat /proc/net/if_inet6
fe800000000000000c79defffe088bd7 10 40 20 80 mesh-vpn
2a032260200f1337000000000000000a 0a 80 00 a0 local-node
fe800000000000000c79defffe088bd2 27 40 20 80    ibss0
2a032260200f0300ae8674fffe00ec00 0b 40 00 00 br-client
fe800000000000000c79defffe088bd0 08 40 20 80   br-wan
2a032260200f0400ae8674fffe00ec00 0b 40 00 00 br-client
2a032260200f1337ae8674fffe00ec00 0b 40 00 00 br-client
fe800000000000000c79defffe088bd3 0c 40 20 80 primary0
fe8000000000000000ffc0fffeffc000 0a 40 20 80 local-node
00000000000000000000000000000001 01 80 10 80       lo
fe80000000000000ae8674fffe00ec02 28 40 20 80  client0
200300c593c57b000c79defffe088bd0 08 40 00 00   br-wan
2a032260200f0100ae8674fffe00ec00 0b 40 00 00 br-client
fe80000000000000ae8674fffe00ec00 0d 40 20 80     bat0
fe80000000000000ae8674fffe00ec00 0b 40 20 80 br-client
fe800000000000000c79defffe088bd4 07 40 20 80 br-mesh_lan

The comment currently explains that the lines have to start with fe80 and then are followed with 37 uninteresting characters. We can therefore ignore the initial 39 characters and not matching lines before going through the next parts of the check:

 80 mesh-vpn
 80    ibss0
 80   br-wan
 80 primary0
 80 local-node
 80  client0
 80     bat0
 80 br-client
 80 br-mesh_lan

The current code then checks that the initial 41 characters are followed by a space and then assumes that a two digit hex number (flags) follows. It will only accept lines when this two digit hex number is not starting with a digit that has the bit 0x4 set.

But the change now allows to have [0-9a-f] after the initial 41 characters. This means that $5 (flags) can be shifted by a single character. This can happen because ifindex ($2) has more than two digits. I would say this will become ugly when more of these checks have to be added - especially because 3 hex digits for $2 (ifindex) is not the actual limit . Instead, maybe it can be changed to:

  • check that line starts with fe80
  • check that $5 is hex number
  • check that ($5 & 0x40) == 0

A problematic line which this change tries to handle is:

fe800000000000000c79defffe088bd4 107 40 20 80 br-new

But it doesn't get this line:

fe800000000000000c79defffe088bd4 1337 40 20 80 br-missing

But it incorrectly accepts line:

fe800000000000000c79defffe088bd4 31337 40 20 40 br-wrong

I would therefore conclude that the change is incorrect

@mwarning
Copy link
Contributor

mwarning commented Apr 6, 2018

Wouldn't this work?

if awk '
		BEGIN { RET=1 }
		/^fe80/ { if ($5 ~ /[012389ab]$/ && $6 == "'"$iface"'") RET=0 }
		END { exit RET }
		' /proc/net/if_inet6;

@ecsv
Copy link
Contributor

ecsv commented Apr 6, 2018

Wouldn't this work?

if awk '
		BEGIN { RET=1 }
		/^fe80/ { if ($5 ~ /[012389ab]$/ && $6 == "'"$iface"'") RET=0 }
		END { exit RET }
		' /proc/net/if_inet6;

No, because this would also match:

fe800000000000000c79defffe088bd4 31337 40 20 40 br-wrong

You may have wanted to use /[012389ab].$/ instead of /[012389ab]$/

@mwarning
Copy link
Contributor

mwarning commented Apr 6, 2018

ah, right. we want to match 0x40, not 0x4

@neocturne
Copy link
Member

I propose this code:

if awk '
	$1 ~ /^fe80/ && $5 ~ /^[012389ab]/ && $6 == "'"$iface"'" { exit 0 }
	END { exit 1 }' /proc/net/if_inet6
' /proc/net/if_inet6;

While not directly related, I also noticed that $6 will not properly match interface names containing whitespace (spaces, tabs...), and I assume interface names with newlines will break the if_inet6 format hopelessly... but maybe we can just ignore the whitespace case because nobody is that insane :)

@adschm
Copy link
Member Author

adschm commented Apr 6, 2018

Haven't had time to follow in detail (my awk knowledge is limited), but will try to do that later. I would test the proposed code and in case of success redo my commit with updated comment as mentioned earlier. Any objections?

@mwarning
Copy link
Contributor

mwarning commented Apr 6, 2018

@adrianschmutzler sounds good!
@NeoRaider whitespaces in interface names are pure evil. ]-:>

@ecsv
Copy link
Contributor

ecsv commented Apr 7, 2018

I was contacted via IRC and asked what comment I am talking about (the one which needs adjustments). This one here: https://github.com/openwrt-routing/packages/blob/448ca58e4cb205eb71f0b889c66e64ce9a028780/alfred/files/alfred.init#L49

@mwarning
Copy link
Contributor

mwarning commented Apr 7, 2018

@ecsv ah, the comment in the source code. Not a comment here in the issue tracker.

@adschm
Copy link
Member Author

adschm commented Apr 9, 2018

I like NeoRaider's solution and it works for me, except that I have to stick with the RET variables and only call exit in the END block. Calling exit directly didn't work for me, don't know why. However, I can live with the variable still being used.

Will update the commit soon.

Occationally /proc/net/if_inet6 contains interface IDs with
three digits. In this case, the regex in wait_for_ll_address()
does not work anymore and alfred is not starting.

This patch changes the evaluation so that fields are used instead
of the mere position by counting characters.

Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>
@adschm
Copy link
Member Author

adschm commented Apr 9, 2018

Patch is updated. Slightly updated the commit message, too.

Since now the solution isn't originating from me any more, should I mention someone in the commit message?

@mwarning
Copy link
Contributor

mwarning commented Apr 9, 2018

@adrianschmutzler no need to include another author for this kind of thing. I don't think @NeoRaider will mind.

@adschm
Copy link
Member Author

adschm commented May 2, 2018

How about merging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants