ifb associated with interface IFACE is still empty

[braveheartleo]

In sqm-scripts - 1.0.2-1, the function get_ifb_associated_with_if() in /usr/lib/sqm/functions.sh does not properly match the associated ifb.

Before the fix:

~# /etc/init.d/sqm stop
SQM: /usr/lib/sqm/stop-sqm: Stopping eth0.2
SQM: ifb associated with interface eth0.2: 
SQM: /usr/lib/sqm/stop-sqm: ifb4eth0.2 shaper deleted
SQM: /usr/lib/sqm/stop-sqm: ifb4eth0.2 interface deleted
SQM: /usr/lib/sqm/stop-sqm: Stopping eth1
SQM: ifb associated with interface eth1: 
SQM: /usr/lib/sqm/stop-sqm: ifb4eth1 shaper deleted
SQM: /usr/lib/sqm/stop-sqm: ifb4eth1 interface deleted

The fix is simple:

--- functions.sh.orig   2015-08-29 03:31:10.000000000 -0800
+++ functions.sh    2015-08-29 03:31:27.000000000 -0800
@@ -65,7 +65,7 @@
 get_ifb_associated_with_if() {
     CUR_IF=$1
     # CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb'[[:digit:]]\+' )
-    CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb'[^)]\+' )    # my editor's syntax coloration is limitied so I need a single quote in this line (between eiditor and s)
+    CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb'[^)]*' )    # my editor's syntax coloration is limitied so I need a single quote in this line (between eiditor and s)
     sqm_logger "ifb associated with interface ${CUR_IF}: ${CUR_IFB}"
     echo ${CUR_IFB}
 }

The + attempts to match the preceeding token ONE OR MORE times. The problem here is that the preceeding token evaluates to a zero match (empty) because we are using the negated character class [^)]. While this may be the case, it is "important to remember that a negated character class still must match a character." [1] In other words, the negated character class must still match a character () in this case), but the resulting token evaluates to zero (empty). Sounds a bit confusing, no? ;) The appropriate repetition to use here, given the preceeding token, is the *, which matches ZERO OR MORE times. [2]

After the fix:

~# /etc/init.d/sqm stop
SQM: /usr/lib/sqm/stop-sqm: Stopping eth0.2
SQM: ifb associated with interface eth0.2: ifb4eth0.2
SQM: /usr/lib/sqm/stop-sqm: ifb4eth0.2 shaper deleted
SQM: /usr/lib/sqm/stop-sqm: ifb4eth0.2 interface deleted
SQM: /usr/lib/sqm/stop-sqm: Stopping eth1
SQM: ifb associated with interface eth1: ifb4eth1
SQM: /usr/lib/sqm/stop-sqm: ifb4eth1 shaper deleted
SQM: /usr/lib/sqm/stop-sqm: ifb4eth1 interface deleted

Cheers, ianp

[1] http://www.regular-expressions.info/charclass.html#negated [2] http://www.regular-expressions.info/repeat.html

[moeller0]

Question, since you recently looked at REs and have a test case that does not work with the current code: why does the original code work for eth0.2 and only fails for eth1?

For me this also works with two sqm instances:

root@nacktmulle:~# /etc/init.d/sqm stop SQM: /usr/lib/sqm/stop-sqm: Stopping pppoe-ge00 SQM: ifb associated with interface pppoe-ge00: ifb4pppoe-ge00 SQM: /usr/lib/sqm/stop-sqm: ifb4pppoe-ge00 shaper deleted SQM: /usr/lib/sqm/stop-sqm: ifb4pppoe-ge00 interface deleted SQM: /usr/lib/sqm/stop-sqm: Stopping se00 SQM: ifb associated with interface se00: ifb4se00 SQM: /usr/lib/sqm/stop-sqm: ifb4se00 shaper deleted SQM: /usr/lib/sqm/stop-sqm: ifb4se00 interface deleted root@nacktmulle:~#

So your fix might be correct, but the explanation is not verbose/complete enough yet, could you elaborate, please?

What we really want is the part between "device ifb" and ") stolen" from the following example output: root@nacktmulle:~# tc -p filter show parent ffff: dev pppoe-ge00 filter protocol all pref 10 u32 filter protocol all pref 10 u32 fh 800: ht divisor 1 filter protocol all pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1 match 00000000/00000000 at 0 action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen index 31 ref 1 bind 1

Note Ithe old code will not match a device called purely "ifb", on purpose. Your version will also match "ifb". The reason is that we can figure out which ifbs are attached to a device, but we can not do the reverse (only by exhaustively searching through all devices), so the idea is only destroy ifbs we created our selves not to shoe the boat. Arguably we should search for strings starting with "ifb4" and also enforce the string to be longer...

Could you please post the output of: tc -p filter show parent ffff: dev eth1 | grep -o -e ifb'[^)]+'

and tc -p filter show parent ffff: dev eth1 | grep -o -e ifb'[^)]*'

Thanks

[braveheartleo]

Hi Sebastian,

Thank you for your inquiry. I looked a little closer into this and these are what I found out:

Let's take this output as our sample:

~# tc -p filter show parent ffff: dev eth1
filter protocol all pref 10 u32 
filter protocol all pref 10 u32 fh 800: ht divisor 1 
filter protocol all pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1 
  match 00000000/00000000 at 0
    action order 1: mirred (Egress Redirect to device ifb4eth1) stolen
    index 4 ref 1 bind 1

Given the sample above, if I feed this into grep -o -e ifb'[^)]*', then I get:

~# tc -p filter show parent ffff: dev eth1 | grep -o -e ifb'[^)]*'
ifb4eth1

So apparently everything looks okay and that the regex pattern is good, right?

But let's use this string this deviceifb ifb is ifb4eth1 stolen as our next sample. If I do this:

~# echo 'this deviceifb ifb is ifb4eth1 stolen' | grep -o -e ifb'[^)]*'
ifb ifb is ifb4eth1 stolen

Yikes! So it looks like you are right and that while grep -o -e ifb'[^)]*' gets the result we want when tc -p filter show parent ffff: dev eth1 is fed into it, the regex does not always hold up true for ALL cases, and as a good practice, we want to form a regex that zeroes in on ifb4 followed by the interface name and nothing more.

Note that I did not go all-out into excluding line breaks, null characters and whatnot, because we are only interested in getting ifb4IFACE from this line action order 1: mirred (Egress Redirect to device ifb4eth1) stolen.

So I revised the regex to grep -o -e ifb4'[^)\ ]*', and this is what I get:

~# echo 'this deviceifb ifb is ifb4eth1 stolen' | grep -o -e ifb4'[^)\ ]*'
ifb4eth1

~# tc -p filter show parent ffff: dev eth1 | grep -o -e ifb4'[^)\ ]*'
ifb4eth1

As you can see above, it works on both samples, and we get exactly ifb4eth1. So the revised fix should now be:

--- functions.sh.orig   2015-08-29 03:31:10.000000000 -0800
+++ functions.sh    2015-08-29 03:31:27.000000000 -0800
@@ -65,7 +65,7 @@
 get_ifb_associated_with_if() {
     CUR_IF=$1
     # CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb'[[:digit:]]\+' )
-    CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb'[^)]\+' )    # my editor's syntax coloration is limitied so I need a single quote in this line (between eiditor and s)
+    CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb4'[^)\ ]*' )    # my editor's syntax coloration is limitied so I need a single quote in this line (between eiditor and s)
     sqm_logger "ifb associated with interface ${CUR_IF}: ${CUR_IFB}"
     echo ${CUR_IFB}
 }

Cheers. ianp

[braveheartleo]

One other thing:

The original code used the ifb<IFACE> naming convention for creating the IFB interface that is used. This is the reason why I stuck with grep -o -e ifb'[^)]*' in the original proposal and only changed the repitition.

But now we are using the ifb4<IFACE> naming convention. So if this is going to be the standardized naming convention from now on, then I guess we can update the code to match ifb4, instead of ifb.

[moeller0]

Hi ianp,

On Aug 29, 2015, at 01:59 , braveheartleo notifications@github.com wrote:

Hi Sebastian,

Thank you for your inquiry. I looked a little closer into this and these are what I found out:

Let's take this output as our sample:

~# tc -p filter show parent ffff: dev eth1 filter protocol all pref 10 u32 filter protocol all pref 10 u32 fh 800: ht divisor 1 filter protocol all pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1 match 00000000/00000000 at 0 action order 1: mirred (Egress Redirect to device ifb4eth1) stolen index 4 ref 1 bind 1

Given the sample above, if I feed this into grep -o -e ifb'[^)]*', then I get:

~# tc -p filter show parent ffff: dev eth1 | grep -o -e ifb'[^)]*' ifb4eth1

So apparently everything looks okay and that the regex pattern is good, right?

But let's use this string this deviceifb ifb is ifb4eth1 stolen as our next sample. If I do this:

~# echo 'this deviceifb ifb is ifb4eth1 stolen' | grep -o -e ifb'[^)]*' ifb ifb is ifb4eth1 stolen

Yikes! So it looks like you are right and that while grep -o -e ifb'[^)]*' gets the result we want when tc -p filter show parent ffff: dev eth1 is fed into it, the regex does not always hold up true for ALL cases, and as a good practice, we want to form a regex that zeroes in on ifb4 followed by the interface name and nothing more.

Double Yikes, why should �tc -p filter show parent ffff: dev eth1� ever return �this deviceifb ifb is ifb4eth1 stolen� my initial goal was to create a regexp that will work with well formed output of tc filter. If the output does not follow the "device ifb4eth1)� pattern, my reasoning was, we can not blindly try our regexp luck. Now are there alternative outputs of �tc -p filter show parent ffff: dev eth1�? I do not know, but if there are I am quite interested to learn. Blindly grep-ing what ever fib-something is in the output seems a bit optimistic.

Note that I did not go all-out into excluding line breaks, null characters and whatnot, because we are only interested in getting ifb4IFACE from this line action order 1: mirred (Egress Redirect to device ifb4eth1) stolen.

So I revised the regex to grep -o -e ifb4'[^)\ ]*', and this is what I get:

~# echo 'this deviceifb is ifb4eth1 stolen' | grep -o -e ifb4'[^)\ ]*' ifb4eth1

Why does the output lack the )?

~# echo 'this deviceifb ifb is ifb4eth1 stolen' | grep -o -e ifb4'[^)\ ]*' ifb4eth1

If tc filter returns such gobbled up output, I believe failing to extract something out of it, as we do no seems justified to me.

~# tc -p filter show parent ffff: dev eth1 | grep -o -e ifb4'[^)\ ]*' ifb4eth1

As you can see above, it works on all samples, and we get exactly ifb4eth1. So the revised fix should now be:

Again your fix might be an improvement (except it will also match ifb4, which is arguable), but the theory why the original regexp failed is still lacking, and I really want to understand the root cause. So why is the closing ) sometimes missing? If you have tc output showing this I would be delighted to see that. T repeat myself, I am not opposed to your change per se, all I want is a better understanding why the original does not work for well formed tc output.

Best Regards Sebastian

--- functions.sh.orig 2015-08-29 03:31:10.000000000 -0800 +++ functions.sh 2015-08-29 03:31:27.000000000 -0800 @@ -65,7 +65,7 @@ get_ifb_associated_with_if() { CUR_IF=$1

CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb'[[:digit:]]+' )

• CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb'[^)]+' ) # my editor's syntax coloration is limitied so I need a single quote in this line (between eiditor and s)
• CUR_IFB=$( tc -p filter show parent ffff: dev ${CUR_IF} | grep -o -e ifb4'[^)\ ]*' ) # my editor's syntax coloration is limitied so I need a single quote in this line (between eiditor and s) sqm_logger "ifb associated with interface ${CUR_IF}: ${CUR_IFB}" echo ${CUR_IFB} }

Cheers. ianp

� Reply to this email directly or view it on GitHub.

[tohojo]

So, one point on this: if we only want to match ifb names that match the name we generate, then there's no reason to do the whole grep dance: we already know the name the ifb is supposed to have (I.e. ifb4$IFACE), so we can just check if that interface exists.

The only reason for going the regex matching is if we want to discover the actual name of an ifb attached to the interface, regardless of its name. But if we don't want to touch ifbs with arbitrary names anyway, why do the matching?

[moeller0]

Hi Toke,

On Aug 29, 2015, at 11:59 , Toke H�iland-J�rgensen notifications@github.com wrote:

So, one point on this: if we only want to match ifb names that match the name we generate, then there's no reason to do the whole grep dance: we already know the name the ifb is supposed to have (I.e. ifb4$IFACE), so we can just check if that interface exists.

The only reason for going the regex matching is if we want to discover the actual name of an ifb attached to the interface, regardless of its name. But if we don't want to touch ifbs with arbitrary names anyway, why do the matching?

Well, if there is no matching fib we do not want to tear it down. I know that stop-sqm tries to do exactly that by creating our default name for an fib, but I am certain that it is better not to try tearing down something that does not exist, instead we should give a warning that no IFB was attached. Note that if one disables ingress shaping, by selecting a bandwidth of zero we have a valid configuration with no IFB, so we should not touch anything.

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[tohojo]

Sure, so check if the ifb exists (I believe checking the return code of 'ip link show dev $ifb' should suffice for that). But why the regex dance?

[moeller0]

Hi Toke,

On Aug 29, 2015, at 12:32 , Toke H�iland-J�rgensen notifications@github.com wrote:

Sure, so check if the ifb exists (I believe checking the return code of 'ip link show dev $ifb' should suffice for that). But why the regex dance?

Basically, because this is what I came up with, I am certain there are other possible implementations (most likely more elegant), but I love redundancy especially for debugging, so I went with the choice that gave me ascii output I can immediately read. Historically I believe this function existed before we enforced �unique� names for our fibs, so by default the names were ifb0 to ifb9 or so, and I really wanted to avoid touching ifbs someone else had setup. Then Dave came up with the observation that IFBs can be created with arbitrary names, and we went and used that to sort of use our own �namespace�.
Now, why you and Ian report that the function sometimes comes up empty, I do not know, but I think would be quite interesting to figure out before embarking on changing the code. I would really like to see the output of the tc filter line for those cases. The �simple� causes might be that the output is missing the closing �)�, but why; or that there is no IFB setup at all, in that case I think we should report that (and potentially check this when we set things up, so we report the error closer to its occurrence). In case tic�s putput is not as stereotypical as I assumed, checking the return code might be more robust (I have not checked whether that works at all), but in the second case we ned to repot more no matter how we detect things.
I am all for evolution, I just want to see the selective pressure first ;)

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[tohojo]

Yup, figured it was a case if history. And perceived a chance to simplify things. Just trying to apply a bit if evolutionary pressure ;)

Also, that and related functions need some cleaning up in any case: figuring out the name if the ifb, deciding what to do with it, and optionally creating and/or destroying it should be logically separate parts of the code, which it isn't currently (at least not always). See for instance the fix I added a couple of commits ago where simply including defaults.sh would try to create an ifb...

[moeller0]

Hi Toke,

On Aug 29, 2015, at 13:22 , Toke H�iland-J�rgensen notifications@github.com wrote:

Yup, figured it was a case if history. And perceived a chance to simplify things. Just trying to apply a bit if evolutionary pressure ;)

Let me be frank to the point of bluntness; without further debugging of the root cause of Ian�s missing output of ifb_associated_with_if I see no selective pressure on this function. Show that it fails and we have a discussion about evolving it further, but until then� (Note, I am still on cerowrt and I believe you and Ian are on openwrt proper so that might be the difference between working always and not working, but even then I want to have better evidence ;) )

Also, that and related functions need some cleaning up in any case: figuring out the name if the ifb, deciding what to do with it, and optionally creating and/or destroying it should be logically separate parts of the code, which it isn't currently (at least not always).

For most of it is quite clear.

See for instance the fix I added a couple of commits ago where simply including defaults.sh would try to create an ifb�

But this was (mostly) the correct thing to do before the refactoring, as we first sourced functions.sh fro the actual qos scripts if I recall correctly. I apologize for not testing the refactoring more diligently�

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[moeller0]

On Aug 29, 2015, at 14:50 , Sebastian Moeller moeller0@gmx.de wrote:

But this was (mostly) the correct thing to do before the refactoring, as we first sourced functions.sh fro the actual qos scripts if I recall correctly. I apologize for not testing the refactoring more diligently�

Please note, I believe the refactoring to be a great thing, so I truly apologize for not spending enough time to try to test all corner cases. It is not like there are enough comments in the scripts to allow easy testing of boundary conditions, again, sorry.

Best Regards Sebastian

[tohojo]

Ah, no worries. We are all amateurs here, and as far as I'm concerned all code sucks and we can but strive to lessen the suckage ;)

Hence, any opportunity to remove code is an opportunity to remove potential suckage. And so should be taken unless there's a good reason not to.

(The above is my tongue-in-cheek way to express the KISS principle, and should not be taken as an insult on anyone's code in particular :) )

[moeller0]

Hi Toke,

On Aug 29, 2015, at 12:32 , Toke H�iland-J�rgensen notifications@github.com wrote:

Sure, so check if the ifb exists (I believe checking the return code of 'ip link show dev $ifb' should suffice for that). But why the regex dance?

Just to be complete the regexp dance actually is the only way I found to figure out which ifb is connected to an interface, the reverse information which interface is the parent to a given fib is not readily available. So all we can do to figure out whether an existing ifb is connected to an interface is query which fibs are connected to said interface, exactly what the function is doing. I believe your test just confirms that an fib exists, but not that is is attached to the relevant interface, so necessary but not sufficient. This certainly needs better diagnostic output though.

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[braveheartleo]

Hi Sebastian,

Double Yikes, why should �tc -p filter show parent ffff: dev eth1� ever return �this deviceifb ifb is ifb4eth1 stolen

It's not really an output of tc -p filter show parent ffff: dev eth1, and I believe I have explicitly called it as a string and nothing more ;). My intention for that string sample was to show that the regexp was well-formed enough to not result in a false-positive match, in the UNLIKELY event that tc should ever produce such gobbled up output.

While you're right that the regexp will match ifb4, this is UNLIKELY to happen because sqm-scripts appends ifb4 before any interface name when it creates IFBs for said interface. I kept the regexp as simple as possible, in keeping with the KISS principle.

Again your fix might be an improvement (except it will also match ifb4, which is arguable), but the theory why the original regexp failed is still lacking, and I really want to understand the root cause.

I haven't found any direct answer to this, but I only found that for examples where a negated character class is used, the + is never used preceeding such class, but only *. See the section beginning with A word of caution. Character classes are... at [1] for example. This might be some "unwritten" (I haven't found any resource explicitly stating such) rule that you can only use * repetition with a negated character class.

ANOTHER NOTE: It looks like tc -p filter show parent ffff: dev $IFACE is not a good basis for extracting associated IFBs, because it outputs nothing WHEN ingress is DEACTIVATED. Could you verify this on your end as well?

Cheers, ianp

[1] http://flex.sourceforge.net/manual/Patterns.html

[moeller0]

Hi Ianp,

On Sep 1, 2015, at 00:31 , braveheartleo notifications@github.com wrote:

Hi Sebastian,

Double Yikes, why should �tc -p filter show parent ffff: dev eth1� ever return �this deviceifb ifb is ifb4eth1 stolen

It's not really an output of tc -p filter show parent ffff: dev eth1, and I believe I have explicitly called it as a string and nothing more ;). My intention for that string sample was to show that the regexp was well-formed enough to not result in a false-positive match, in the UNLIKELY event that tc should ever produce such gobbled up output.

Ah, here is our disconnect, n the case of a gobbled output I intended the grep to fail. The design goals were:

1) to extract the IFB name from a well-formed tc output only, (if the output is actually variant, I would rather add more explicit greps for each known variant than trying to match blindly) Effectively the grep looks for the closing parenthesis but omits it from its output. 2) to not match a blank unadorned IFB, I admit the reasoning for this is disputable; initially our IFB was called ifbN with 0 <= N <= 9, and I did not want to clobber any other IFB we had not set up our selves. (qos-script for example uses ifb0, I believe). But this means that "ifb)” is not matched on purpose! I believe the stuff you quoted basically just means that the grep-line will come up empty if there is no character in the negated class, pretty much what I wanted.

While you're right that the regexp will match ifb4, this is UNLIKELY to happen because sqm-scripts appends ifb4 before any interface name when it creates IFBs for said interface. I kept the regexp as simple as possible, in keeping with the KISS principle.

The traditional way is to use ifbN as names for IFBs, so “ifb4)” really should not be matched in my book. I should mention that a single IFB can be associated with several interfaces, and if a user did something elaborate using this fact I do not think SQM should step on their toes, pretending it knows better.

Again your fix might be an improvement (except it will also match ifb4, which is arguable), but the theory why the original regexp failed is still lacking, and I really want to understand the root cause.

I haven't found any direct answer to this, but I only found that for examples where a negated character class is used, the + is never used preceeding such class, but only . See the section beginning with A word of caution. Character classes are... at [1]. This might be some "unwritten" (I haven't found any resource explicitly stating such) rule that you can only use \ repetition with a negated character class.

Well, I believe in your case the original grep works fine for eth0.2, so it can not be all wrong, really I want proof that the original grep fails at real tc output before changing things. I believe there might be a real bug we should investigate, changing the grep without proof that it fails for valid tc output feels like papering over instead of fixing…
Also I tested the grep line under macosx, opens use, cerowrt and got the expected results, so I believe that the original grep fulfills my design target while your proposals fail to meet design goal 2) above. Now we can discuss whether 2) is still a valid goal, but that is a different discussion than the one we are having now.

ANOTHER NOTE: It looks like tc -p filter show parent ffff: dev $IFACE is not a good basis for extracting associated IFBs, because it outputs nothing WHEN ingress is deactivated. Could you verify this on your end as well?

If ingress is deactivated there is no association between $IFACE and the IFB so get_ifb_associated_with_if reports no association, which in my book is spot on, no? If you have a test script showing a failure I am happy to try that. But really as far as I can see get_ifb_associated_with_if() is doing pretty much what it was supposed to do ;)

Best Regards

Cheers, ianp

[1] http://flex.sourceforge.net/manual/Patterns.html

— Reply to this email directly or view it on GitHub.

[moeller0]

As a more productive proposal I will play around with the different grep invocations to better understand their failure modes...

[moeller0]

SO here I go:

CEROWRT: How does well-formed tc output look like: root@nacktmulle:~# tc -p filter show parent ffff: dev pppoe-ge00 filter protocol all pref 10 u32 filter protocol all pref 10 u32 fh 800: ht divisor 1 filter protocol all pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1 match 00000000/00000000 at 0 action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen index 21 ref 1 bind 1

Now feed well-formed input to grep:
root@nacktmulle:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00 root@nacktmulle:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]*' ifb4pppoe-ge00

So both grep lines work for well-formed input, good.

Now look for unadorned fib name: root@nacktmulle:~# echo "action order 1: mirred (Egress Redirect to device ifb) stolen" | grep -o -e ifb'[^)]+' root@nacktmulle:~# echo "action order 1: mirred (Egress Redirect to device ifb) stolen" | grep -o -e ifb'[^)]*' ifb

"+" works as I intended, "*" does not.

And now for corrupted tc output (missing parenthesis) root@nacktmulle:~# root@nacktmulle:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00 stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00 stolen root@nacktmulle:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00 stolen" | grep -o -e ifb'[^)]*' ifb4pppoe-ge00 stolen

Unfortunately both produce gunk instead of returning nothing.

It looks osx behaves identically: computer:~ user$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00 computer:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]' ifb4pppoe-ge00 computer:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00 stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00 stolen computer:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00 stolen" | grep -o -e ifb'[^)]' ifb4pppoe-ge00 stolen computer:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb) stolen" | grep -o -e ifb'[^)]+' computer:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb) stolen" | grep -o -e ifb'[^)]*' ifb

My take on this is the current grep is closer to the intended goal than the replacement, but both fail to reject mis-formed inputs; so back to the drawing board. But until then both will work fine

Again I am all for fixing bugs, just want to be confident we have a bug at our hands; currently this looks more like discussing which color to paint the bike-shed with ;)

Best Regards Sebastian

[braveheartleo]

Please note I tried to respond to your comment inline, before realizing that this would edit the original comment. I did not intend that, and hope I have restored your comment to its original state. oops.

And here is where our disconnect apparently lies.

Taking your samples from above, on CHAOS CALMER (Bleeding Edge, r46734):

root@AG300H:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+'

root@AG300H:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]*'
ifb4pppoe-ge00

On Ubuntu 15.04:

ianp@xbox:~$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+'

ianp@xbox:~$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]*'
ifb4pppoe-ge00

Anyway it is rather pointless to go any further if you're seeing one thing and i'm seeing something completely different.

If ingress is deactivated there is no association between $IFACE and the IFB so get_ifb_associated_with_if reports no association, which in my book is spot on, no? If you have a test script showing a failure I am happy to try that. But really as far as I can see get_ifb_associated_with_if() is doing pretty much what it was supposed to do ;)

I'm still getting around understanding traffic shaping, so excuse that momentary lack of knowledge on my part.

My take on this is the current grep is closer to the intended goal than the replacement, but both fail to reject mis-formed inputs; so back to the drawing board. But until then both will work fine

Don't you think that statement is rather heavily biased, considering that the current grep is 1) YOUR CODE, and 2) YOU COMPLETELY IGNORED my revised fix, which was to use grep -o -e ifb4'[^)\ ]*'.

If you're gonna argue that now it matches an interface named ifb4, then show me how could an interface be named ifb4 THAT IS NOT AN INTENTIONAL SABOTAGE, NOT EVEN A MISTAKE OR OTHERWISE, because it appears you're saying that the code is intentionally trying to match an interface named ifb4, but that is not the case.

It is an accepted compromise that is FULLY DEPENDENT on a well-formed tc output, and properly named interface. Your code is dependent on such premises as well. Like Toke said, "all code sucks".

Why should I go through all the trouble trying to revise my code, even providing actual use of the revised code, if it wouldn't be read in the first place? Never again.

This is getting tiresome, and I wouldn't have reported anything if there wasn't an OBSERVED PROBLEM ON MY END.

One thing's for sure, I won't be touching your code again.

[moeller0]

As much as I would like it this issue is not settled. We still have the observation that get_ifb_associated_with_if occasionally comes up empty. Potential reasons for this are: 1) the regexp currently used in get_ifb_associated_with_if fails to extract a well-formed IFB name from tc -p filter show parent ffff: dev ${CUR_IF}

2) associating the successfully created IFB device with $IFACE might have failed.

3) the initial creation of the IFB ifb4${IFACE} might have failed.

All three points are well worth investigating. I want to explicitly put 1) on the table, I do not claim that the current regular expression might not be at fault.

Ianp, I would be delighted if you would continue helping fix this issue, for me this is not about habving the last word, but about making sqm-scrips more robust.

I believe all three points need addressing to actually make the code more robust.

Best Regards Sebastian

[moeller0]

Hi Ianp,

     sorry for getting you all upset. I apologize for being to grumpy, I really value your input and I do acknowledge that we have an issue that needs fixing, many thanks for reporting. I am however not confident enough that we have localized the root cause of said issue and hence would like to keep on investigating. I do need your help for that as it seems on cerowrt I can recreate the failures (yet).

On Sep 2, 2015, at 09:59 , braveheartleo notifications@github.com wrote:

And here is where our disconnect apparently lies.

Taking your samples from above, on CHAOS CALMER (Bleeding Edge, r46734):

root@AG300H:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+'

root@AG300H:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]*' ifb4pppoe-ge00

On Ubuntu 15.04:

ianp@xbox:~$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+'

ianp@xbox:~$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]*' ifb4pppoe-ge00

Anyway it is rather pointless to go any further if you're seeing one thing and i'm seeing something completely different.

Ah, I screwed up, in functions.sh the regexp is grep -o -e ifb'[^)]\+' (just have a look yourself, this is not a recent change) so I forgot to put the backslash here in the comments. I am sorry for that. 
Here is the output of ubuntu 14.04:

user@ubuntu1404:~$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' user@ubuntu1404:~$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00

And of macosx 10.9: GNU grep

macosx:~ user$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' macosx:~ user$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00 And of macosx 10.9: BSD grep macosx:~ user$ /usr/bin/grep -V grep (BSD grep) 2.5.1-FreeBSD macosx:~ user$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | /usr/bin/grep -o -e ifb'[^)]+' macosx:~ user$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | /usr/bin/grep -o -e ifb'[^)]+' ifb4pppoe-ge00

And of busy box (cerowrt 3.10.50-1):

user@cerowrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' user@ cerowrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00

And of opensuse 13.2

user@opensuse ppy:~/CODE/sqm-scripts/src> echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' user@opensuse:~/CODE/sqm-scripts/src> echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00

And for good measure cygwin:

user@cygwin ~ $ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+�

user@ cygwin ~ $ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00

     As far as I can tell this works okay. (It fails when the closing parenthesis is missing, so needs refinements the �back to the drawing board� was aimed at myself). These are four different grep versions (3 different GNU greps plus busybox�s) that all seem to work. Versus the one in CC that does not. 
Could you do me the favor of running the exact same invocations I tested on these 4 machines above on your router under your CC build again, please? I am quite data driven, so please humor me and show the output of �tc -p filter show parent ffff: dev eth0.2� as well, when sqm is activated.

If both invocations (the typed one from above as well as the one ref;letting the code in functions.sh) return nothing, I agree with your interpretation that the regexp should change (to work around CC�s aberrant grep behavior ;) ).

If ingress is deactivated there is no association between $IFACE and the IFB so get_ifb_associated_with_if reports no association, which in my book is spot on, no? If you have a test script showing a failure I am happy to try that. But really as far as I can see get_ifb_associated_with_if() is doing pretty much what it was supposed to do ;)

I'm still getting around understanding traffic shaping, so excuse that momentary lack of knowledge on my part.

    No need to apologize. I tried to explain why your observation is correct and does not diminish the way get_ifb_associated_with_if works. Clearly the function is not well-named (I am not too good at naming) nor well commented, sorry.

My take on this is the current grep is closer to the intended goal than the replacement, but both fail to reject mis-formed inputs; so back to the drawing board. But until then both will work fine

Don't you think that statement is rather heavily biased, considering that the current grep is 1) YOUR CODE, and 2) YOU COMPLETELY IGNORED my revised fix, which was to use grep -o -e ifb4'[^)\ ]*�.

Please let me address your point sequentially. Regarding 1), if you see above the disputed regexp seems to to the right thing on all machines I have access to, so please bear with me and show me why the apparent failure of CC�s busybox implementation of grep to process this regexp (which as shown above can not be totally broken) is a failure of �my code� instead of arsine behavior of this specific grep? That said, I will surely take your fixed regexp as this seems works well enough everywhere including your CC version.
Regarding 2) I am not sure in how far your revised regexp (which I copy and pasted from your paragraph again, as I do not trust myself to write this myself error-free) changes things (for simplicities sake just run with GNU grep on macosx):

macosx:~ user$ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb4'[^)\ ]' ifb4pppoe-ge00 macosx:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00 stolen" | grep -o -e ifb4'[^)\ ]' ifb4pppoe-ge00 macosx:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb4 stolen" | grep -o -e ifb4'[^)\ ]' ifb4 macosx:~ user $ echo "action order 1: mirred (Egress Redirect to device ifb4) stolen" | grep -o -e ifb4'[^)\ ]' ifb4

I do note that this has the arguable advantage of getting a single word result if the ) is missing, as compared to the �ifb4 stolen� the initial \+ grep version had. (I would prefer failure in that case though, as if tic�s output is different from what we expect blindly gripping something seems overly optimistic; I do note that the current code has this issue already and fixing it might not be in the realm of changing the single regexp we are discussing right now).

If you're gonna argue that now it matches an interface named ifb4, then show me how could an interface be named ifb4 THAT IS NOT AN INTENTIONAL SABOTAGE, NOT EVEN A MISTAKE OR OTHERWISE, because it appears you're saying that the code is intentionally trying to match an interface named ifb4, but that is not the case.

Any openwrt user is free to configure its router after her own fancy, if a user decided to use ifb devices for what ever purpose these typically follow the ifbN naming convention, similar to ethN. So I see no reason why the name ifb4 is guaranteed to be available exclusively for us. My intention was being a good openwrt citizen and avoid trampling over peoples custom  configurations just because I would now better. In the end tis should actually allow co-existence of activated qos-scripts and sqm-scripts on the same router (though not on the same IFACE ;) ), not sure whether that is a reasonable goal.
All of this happened before Dave Taeht pointed out that ifb devices can be created with arbitrary names (<= 15 characters) so at a time when all ifb where called ifbN, and when it was really not guaranteed that a given N was under sqm-scrpt�s control. With the names ifb devices clashes are way less likely, and Tokes proposal still stands to check the existence of the ifb4${IFACE} device and its type before taking it down instead. But even then we need to check somehow whether our ifb was �associated� with its IFACE, so basically the functionality we are discussing here.

It is an accepted compromise that is FULLY DEPENDENT on a well-formed tc output, and properly named interface. Your code is dependent on such premises as well. Like Toke said, "all code sucks�.

Sure and no code is sacred (and typically I understand my code as a prototype that Toke will need to refine later). I believe I have been hedging my bets in our conversation by repeatedly writing stuff along the lines of �your change might be an improvement�. All I want is a clear demonstration of the failure here in the comments or a commit message so that it is clear why the change is good. I will have a look at discarding mis-formed tc output separately, I agree that is a different issue. (This is also why I partly ignored the regexp documentation lil you posted, if the documentation disagrees with the implementation (and I am not about to change the implementation) it is the code that wins.)

Why should I go through all the trouble trying to revise my code, even providing actual use of the revised code, if it wouldn't be read in the first place? Never again.

I am under the firm impression, that both of us are not the most attentive readers (amply demonstrated in this and other discussions), so from my part, please, take me apology for that and promise to read closer and think longer before responding. So peace?

This is getting tiresome, and I wouldn't have reported anything if there wasn't an OBSERVED PROBLEM ON MY END.

I believe that I had acknowledged that observed problem pretty much from the start, the disagreement comes over the theory about the root cause of the problem. You seem to have found a solution that works for you, assuming that this is representative of all devices running CC your fix might be the right thing to do. What your fix fails at is explaining why the failure does not seem to manifest in the bunch of non-CC tests I have been sprinkling through the discussion (see above). I am sorry, but I believe that question is worth answering before declaring success and moving on.

One thing's for sure, I won't be touching your code again.

Oh, come on, if at all its Toke�s code, not mine. So if you convince Toke a change is worth while, it is in, independent on my opinion, I am not a gate keeper for this project (I will try to argue about the merit of a proposed change as I will partly deal with potential fall-out). You can completely side-step me and ignore my feed-back; so if you dislike my demeanor ignore me, but please keep contributing, sqm-scripts needs people actually looking at it closely.

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[moeller0]

Okay, so I tried harder: I set up a virtual box VM using openwrt-15.05-rc3-x86-generic-combined-ext4.img as basis. After a bit of fiddling with the network settings this booted up. This is based on CC-RC3 r46163.

With this image booted both regular expressions we are discussing here work when issued from the command line.

root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb4'[^)\ ]' ifb4pppoe-ge00 root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4) stolen" | grep -o -e ifb4'[^)\ ]' ifb4 root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4pppoe-ge00) stolen" | grep -o -e ifb'[^)]+' ifb4pppoe-ge00 root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4) stolen" | grep -o -e ifb'[^)]+' ifb4 root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4) stolen" | grep -o -e ifb4'[^)]+' root@OpenWrt:~#

Also note the following thgis is with the most recent version of functions.sh which kept the initial regexp but learned to report the current situation more verbosely: root@OpenWrt:~# uname -a Linux OpenWrt 3.18.17 #1 Fri Jul 3 21:07:06 CEST 2015 i686 GNU/Linux root@OpenWrt:~# tc -d qdisc qdisc fq_codel 0: dev eth0 root refcnt 2 limit 1024p flows 1024 quantum 300 target 5.0ms interval 100.0ms ecn qdisc fq_codel 0: dev eth1 root refcnt 2 limit 1024p flows 1024 quantum 300 target 5.0ms interval 100.0ms ecn root@OpenWrt:~# /etc/init.d/sqm stop root@OpenWrt:~# cat /etc/config/sqm

config queue 'eth1' option enabled '1' option download '10000' option upload '10000' option qdisc 'fq_codel' option script 'simple.qos' option qdisc_advanced '0' option linklayer 'none' option interface 'eth1'

config queue option qdisc 'fq_codel' option script 'simple.qos' option squash_dscp '1' option squash_ingress '1' option ingress_ecn 'ECN' option egress_ecn 'NOECN' option linklayer 'none' option enabled '1' option interface 'eth0' option download '20000' option upload '20000' option qdisc_advanced '1' option qdisc_really_really_advanced '0'

root@OpenWrt:~# /etc/init.d/sqm start SQM: /usr/lib/sqm/start-sqm: Starting eth1 SQM: /usr/lib/sqm/start-sqm: Queue Setup Script: simple.qos SQM: QDISC htb is useable. SQM: QDISC fq_codel is useable. SQM: Starting simple.qos SQM: ifb associated with interface eth1: SQM: Currently no ifb is associated with eth1, this is normal during starting of the sqm system. SQM: Squashing differentiated services code points (DSCP) from ingress. SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: egress shaping activated SQM: QDISC ingress is useable. SQM: Do not perform DSCP based filtering on ingress. (1-tier classification) SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: ingress shaping activated SQM: /usr/lib/sqm/start-sqm: Starting eth0 SQM: /usr/lib/sqm/start-sqm: Queue Setup Script: simple.qos SQM: QDISC htb is useable. SQM: QDISC fq_codel is useable. SQM: Starting simple.qos SQM: ifb associated with interface eth0: SQM: Currently no ifb is associated with eth0, this is normal during starting of the sqm system. SQM: Squashing differentiated services code points (DSCP) from ingress. SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: egress shaping activated SQM: QDISC ingress is useable. SQM: Do not perform DSCP based filtering on ingress. (1-tier classification) SQM: get_limit: CURLIMIT: 1001 SQM: get_target defaulting to auto. SQM: ingress shaping activated root@OpenWrt:~# tc -d qdisc qdisc htb 1: dev eth0 root refcnt 2 r2q 10 default 12 direct_packets_stat 0 ver 3.17 direct_qlen 1000 qdisc fq_codel 110: dev eth0 parent 1:11 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms qdisc fq_codel 120: dev eth0 parent 1:12 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms qdisc fq_codel 130: dev eth0 parent 1:13 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms qdisc ingress ffff: dev eth0 parent ffff:fff1 ---------------- qdisc htb 1: dev eth1 root refcnt 2 r2q 10 default 12 direct_packets_stat 0 ver 3.17 direct_qlen 1000 qdisc fq_codel 110: dev eth1 parent 1:11 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms ecn qdisc fq_codel 120: dev eth1 parent 1:12 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms ecn qdisc fq_codel 130: dev eth1 parent 1:13 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms ecn qdisc ingress ffff: dev eth1 parent ffff:fff1 ---------------- qdisc htb 1: dev ifb4eth1 root refcnt 2 r2q 10 default 10 direct_packets_stat 0 ver 3.17 direct_qlen 32 qdisc fq_codel 110: dev ifb4eth1 parent 1:10 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms ecn qdisc htb 1: dev ifb4eth0 root refcnt 2 r2q 10 default 10 direct_packets_stat 0 ver 3.17 direct_qlen 32 qdisc fq_codel 110: dev ifb4eth0 parent 1:10 limit 1001p flows 1024 quantum 300 target 5.0ms interval 100.0ms ecn root@OpenWrt:~# /etc/init.d/sqm stop SQM: /usr/lib/sqm/stop-sqm: Stopping eth0 SQM: ifb associated with interface eth0: ifb4eth0 SQM: /usr/lib/sqm/stop-sqm: ifb4eth0 shaper deleted SQM: /usr/lib/sqm/stop-sqm: ifb4eth0 interface deleted SQM: /usr/lib/sqm/stop-sqm: Stopping eth1 SQM: ifb associated with interface eth1: ifb4eth1 SQM: /usr/lib/sqm/stop-sqm: ifb4eth1 shaper deleted SQM: /usr/lib/sqm/stop-sqm: ifb4eth1 interface deleted root@OpenWrt:~#

Now, I am really at a loss, I seem to be totally unable to recreate the failure even using CC-RC3 x86... and a dual shaper set-up similar to yours. Could it be that we both work with different revisions of sqm-scripts? Or is this a difference between platforms, so x86 versus the architecture of your router? Heck, I notice you use VLANs for eth0, so I will try to recreate that as well

Or is this s change introduced between my CC r46163 and your CC r46734? Is this the track towards CC final or the branch that will develop into DD?

Okay using yesterday's snapshot I see a similar failure.

root@OpenWrt:~# cat /etc/banner

---

| |.-----.-----.-----.| | | |.----.| | | - || | -| || | | || || | |_____|| |___||||____||| |__| |__| W I R E L E S S F R E E D O M

---

CHAOS CALMER (Bleeding Edge, r46760)

---

• 1 1/2 oz Gin Shake with a glassful
• 1/4 oz Triple Sec of broken ice and pour
• 3/4 oz Lime Juice unstrained into a goblet.
• 1 1/2 oz Orange Juice

• 1 tsp. Grenadine Syrup

  root@OpenWrt:~# uname -a Linux OpenWrt 3.18.20 #1 Tue Sep 1 03:06:50 UTC 2015 i686 GNU/Linux root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)\ ]' ifb4eth0 root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth1) stolen" | grep -o -e ifb'[^)\ ]' ifb4eth1 root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth1) stolen" | grep -o -e ifb'[^)]+' root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)]+' root@OpenWrt:~#

So this issue is restricted to openWrt > CC-RC3, I hope you understand that I am a bit grumpy that I had to to all the heavy lifting of localizing this myself...

Now the interesting question obviously is why?

busy box seems similar (same version different build): root@OpenWrt:~# cat /etc/banner

---

| |.-----.-----.-----.| | | |.----.| | | - || | -| || | | || || | |_____|| |___||||____||| |__| |__| W I R E L E S S F R E E D O M

---

CHAOS CALMER (Bleeding Edge, r46760)

---

• 1 1/2 oz Gin Shake with a glassful
• 1/4 oz Triple Sec of broken ice and pour
• 3/4 oz Lime Juice unstrained into a goblet.
• 1 1/2 oz Orange Juice

• 1 tsp. Grenadine Syrup

  root@OpenWrt:~# busybox BusyBox v1.23.2 (2015-09-01 02:43:46 UTC) multi-call binary.

  ---

  | |.-----.-----.-----.| | | |.----.| | | - || | -| || | | || || | |_____|| |___||||____||| |__|

  |__| W I R E L E S S F R E E D O M

  CHAOS CALMER (15.05-rc3, r46163)

• 1 1/2 oz Gin Shake with a glassful
• 1/4 oz Triple Sec of broken ice and pour
• 3/4 oz Lime Juice unstrained into a goblet.
• 1 1/2 oz Orange Juice

• 1 tsp. Grenadine Syrup

  root@OpenWrt:~# busybox BusyBox v1.23.2 (2015-06-18 13:39:39 CEST) multi-call binary.

tc binary is the same; BUT Bleeding Edge uses MULS while CC-RC3 is still on uCLIB.

This could be the smoking gun, or not, I will post pone investigation until some other time. All that is left figuring out whether/how the clib switch affected the regexp behavior. And then the fix might well be your refined regexp pattern.

Best Regards Sebastian

[tohojo]

Seems plausible that the libc switch would affect the regex matching. The question is whether it's a bug in musl or not...

Your examples above at all missing the escape of the + sign; was that deliberate?

[moeller0]

Hi Toke,

On Sep 2, 2015, at 22:16 , Toke H�iland-J�rgensen notifications@github.com wrote:

Seems plausible that the libc switch would affect the regex matching. The question is whether it's a bug in musl or not�

Good question, for all I know musk could be simply stricter in what it accepts than the other clibs. But then again the difference in behavior might be unrelated to the switch to musk, currently that is just a nice but unconfirmed hypothesis.

Your examples above at all missing the escape of the + sign; was that deliberate?

Not sure I get you, could you elaborate how an escaped + would look like?

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[tohojo]

moeller0 notifications@github.com writes:

Not sure I get you, could you elaborate how an escaped + would look like?

An escaped + is + :)

-Toke

[moeller0]

Hi Toke,

On Sep 2, 2015, at 22:16 , Toke H�iland-J�rgensen notifications@github.com wrote:

Seems plausible that the libc switch would affect the regex matching. The question is whether it's a bug in musl or not...

Your examples above at all missing the escape of the + sign; was that deliberate?

I believe you are talking about the difference between �+� and �\+�? I accidentally typo�ed the regexp when I asked Ian to test on his machine and I wanted to carry the broken version along so it was obvious that \+ actually works, while the typo being a typo does not.

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[tohojo]

Right. I may be missing it (github seems to have garbled your paste quite a bit), but where's the part where you test the actual + ?

[moeller0]

On Sep 2, 2015, at 22:26 , Toke H�iland-J�rgensen notifications@github.com wrote:

Right. I may be missing it (github seems to have garbled your paste quite a bit), but where's the part where you test the actual + ?

� Reply to this email directly or view it on GitHub.

MUSL-based:

root@OpenWrt:~# cat /etc/banner

---

| |.-----.-----.-----.| | | |.----.| | | - || | -| || | | || || | |_____|| |___||||____||| |__| |__| W I R E L E S S F R E E D O M

---

CHAOS CALMER (Bleeding Edge, r46760)

---

• 1 1/2 oz Gin Shake with a glassful
• 1/4 oz Triple Sec of broken ice and pour
• 3/4 oz Lime Juice unstrained into a goblet.
• 1 1/2 oz Orange Juice

• 1 tsp. Grenadine Syrup

  root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)]+� root@OpenWrt:~# root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)\ ]*' ifb4eth0 root@OpenWrt:~# root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)]+' root@OpenWrt:~#

and UCLIB-based:

root@OpenWrt:~# cat /etc/banner

---

| |.-----.-----.-----.| | | |.----.| | | - || | -| || | | || || | |_____|| |___||||____||| |__| |__| W I R E L E S S F R E E D O M

---

CHAOS CALMER (15.05-rc3, r46163)

---

• 1 1/2 oz Gin Shake with a glassful
• 1/4 oz Triple Sec of broken ice and pour
• 3/4 oz Lime Juice unstrained into a goblet.
• 1 1/2 oz Orange Juice

• 1 tsp. Grenadine Syrup

  root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)]+� ifb4eth0 root@OpenWrt:~# root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)]+' root@OpenWrt:~# root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -e ifb'[^)\ ]*' ifb4eth0 root@OpenWrt:~#

I hope the direct mail will keep it intact.

Best Regards Sebastian

[tohojo]

Okay, playing around with a snapshot myself, it seems the '+' match is broken entirely in grep unless passing the -E switch to grep:

# echo meep | grep -o -e 'm[e]+'
# echo meep | grep -o -e 'm[e]\+'
# echo meep | grep -o -E 'm[e]+'
mee
# echo meep | grep -o -e 'm[e]\{1,\}'
mee

The last one I got from looking at the actual source: http://git.musl-libc.org/cgit/musl/tree/src/regex/regcomp.c#n768

Seems they do not include the '+' match in the basic regular expressions at all. So there you go :)

[moeller0]

Hi Toke,

On Sep 2, 2015, at 23:08 , Toke H�iland-J�rgensen notifications@github.com wrote:

Okay, playing around with a snapshot myself, it seems the '+' match is broken entirely in grep unless passing the -E switch to grep:

echo meep | grep -o -e 'm[e]+'

echo meep | grep -o -e 'm[e]+'

echo meep | grep -o -E 'm[e]+'

mee

echo meep | grep -o -e 'm[e]{1,}'

mee

The last one I got from looking at the actual source: http://git.musl-libc.org/cgit/musl/tree/src/regex/regcomp.c#n768

Seems they do not include the '+' match in the basic regular expressions at all. So there you go :)

After doing a bit of reading on this myself, I believe that might be one of the non-standard GNU extensions�

But you are right:

echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -E ifb'[^)]+�

seems to work: CHAOS CALMER (15.05-rc3, r46163) root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -E ifb'[^)]+' ifb4eth0

CHAOS CALMER (Bleeding Edge, r46760) root@OpenWrt:~# echo "action order 1: mirred (Egress Redirect to device ifb4eth0) stolen" | grep -o -E ifb'[^)]+' ifb4eth0

Then again, I believe Ian�s regexp also works for all grep/libc versions I tested.

Now, since I have your attention, do you have a regexp that will only match a pattern like

"ifbN)"

with N being a sequence >= 1 non-white space characters. Especially

it should not match on �ifb)" and "ifbN � so white space instead of a closing parenthesis

it also should not put the ) into its output...

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[tohojo]

Well, ifb[^) ]+ is "ifbN" where N is one or more characters that can be anything except a space or a ).

If you want to be fancy and exclude any kind of whitespace, you can use ifb[^)[:space:]]+.

[tohojo]

Or you could go the other way and whitelist expected characters: ifb[[:alnum:]_-]+ would be alpha-numeric characters and _ and -.

[moeller0]

Hi Toke,

On Sep 2, 2015, at 23:26 , Toke H�iland-J�rgensen notifications@github.com wrote:

All nice,

Or you could go the other way and whitelist expected characters: ifb[[:alnum:]-]+ would be alpha-numeric characters and and -.

But what is missing is the not match if no ) terminating the ifb name ;) I guess that just needs a second test in this function.

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[tohojo]

Ah, so you want to test that the ) is there, but not actually output it?

[moeller0]

Hi Toke,

I just committed a change to the regexp so that it works on CC and post CC.

Hopefully this stays sane in the future.

BTW, this is what I mean with figuring out the root cause of unexplained behavior as compared to "papering over�, it is not so much the fix as the reasoning behind the fix�

Best Regards Sebastian

On Sep 2, 2015, at 23:26 , Toke H�iland-J�rgensen notifications@github.com wrote:

Or you could go the other way and whitelist expected characters: ifb[[:alnum:]-]+ would be alpha-numeric characters and and -.

� Reply to this email directly or view it on GitHub.

[moeller0]

Hi Toke,

On Sep 2, 2015, at 23:44 , Toke H�iland-J�rgensen notifications@github.com wrote:

Ah, so you want to test that the ) is there, but not actually output it?

That would be nice, to allow easy failure if tc�s output does not look like we expect. But so far we had no issue with tc�s output, so maybe this is overdoing things...

Best Regards Sebastian

� Reply to this email directly or view it on GitHub.

[tohojo]

moeller0 notifications@github.com writes:

That would be nice, to allow easy failure if tc�s output does not look like we expect. But so far we had no issue with tc�s output, so maybe this is overdoing things...

I'd say it was overdoing it. And you can't do a match (not with grep, in any case) and not output it as part of the -o output.

What you can do is remove it afterwards:

# echo 'test)' | grep -o -E 'te[st]+)' | tr -d ')'
test

-Toke

[tohojo]

Also, since the change you checked in should fix the issue, I guess we can close it :)

[moeller0]

Hi Toke,

On Sep 2, 2015, at 23:54 , Toke Høiland-Jørgensen notifications@github.com wrote:

moeller0 notifications@github.com writes:

That would be nice, to allow easy failure if tc�s output does not look like we expect. But so far we had no issue with tc�s output, so maybe this is overdoing things...

I'd say it was overdoing it.

Okay, let’s ignore this until we have new issues there, currently get_ifb_associated_with_if seems to work from cerowrt to CC bleeding edge, so we should be fine.

Best Regards Sebastian

And you can't do a match (not with grep, in any case) and not output it as part of the -o output.

What you can do is remove it afterwards:

echo 'test)' | grep -o -E 'te[st]+)' | tr -d ')'

test

-Toke — Reply to this email directly or view it on GitHub.