| Message ID | 20150405121104.GD23433@acer.localdomain |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
Am 05.04.2015 um 14:11 schrieb Patrick McHardy: > On 05.04, Patrick McHardy wrote: >> Basically this involves splitting the expression types into lhs (non-const) >> and rhs (const) parts. Keywords on the RHS side can be caught using an >> error statement and deferred to resolution during runtime. Sounds like trial and error. ;) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05.04, Alexander Holler wrote: > Am 05.04.2015 um 14:11 schrieb Patrick McHardy: > > On 05.04, Patrick McHardy wrote: > > >> Basically this involves splitting the expression types into lhs (non-const) > >> and rhs (const) parts. Keywords on the RHS side can be caught using an > >> error statement and deferred to resolution during runtime. > > Sounds like trial and error. ;) The approach is, the patch isn't, it changes the grammar to have these kinds of errors in a defined state. The patch I sent however is, but I'm quite sure i understand the implications. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 April 2015 at 14:11, Patrick McHardy <kaber@trash.net> wrote: > > This needs a lot of testing though since it has the potential to break > things quite badly. Since I'm busy, maybe someone else wants to start by > running the testsuite with this patch applied. I was curious and ran the testsuite with your patch. It's OK. best regards.
Am 06.04.2015 um 03:51 schrieb Patrick McHardy: > On 05.04, Alexander Holler wrote: >> Am 05.04.2015 um 14:11 schrieb Patrick McHardy: >>> On 05.04, Patrick McHardy wrote: >> >>>> Basically this involves splitting the expression types into lhs (non-const) >>>> and rhs (const) parts. Keywords on the RHS side can be caught using an >>>> error statement and deferred to resolution during runtime. >> >> Sounds like trial and error. ;) > > The approach is, the patch isn't, it changes the grammar to have > these kinds of errors in a defined state. The patch I sent > however is, but I'm quite sure i understand the implications. Just to mention it, there is still the possibility to define and use keywords for all the icmp type names. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.04.2015 um 10:44 schrieb Alexander Holler: > Am 06.04.2015 um 03:51 schrieb Patrick McHardy: >> On 05.04, Alexander Holler wrote: >>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy: >>>> On 05.04, Patrick McHardy wrote: >>> >>>>> Basically this involves splitting the expression types into lhs >>>>> (non-const) >>>>> and rhs (const) parts. Keywords on the RHS side can be caught using an >>>>> error statement and deferred to resolution during runtime. >>> >>> Sounds like trial and error. ;) >> >> The approach is, the patch isn't, it changes the grammar to have >> these kinds of errors in a defined state. The patch I sent >> however is, but I'm quite sure i understand the implications. > > Just to mention it, there is still the possibility to define and use > keywords for all the icmp type names. Preferable with names as used in icmp.h and icmpv6.h. As these are defines in C-headers, there is very high probability that these names are unique, even across a large number of different sets of type or other names. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.04.2015 um 11:01 schrieb Alexander Holler: > Am 06.04.2015 um 10:44 schrieb Alexander Holler: >> Am 06.04.2015 um 03:51 schrieb Patrick McHardy: >>> On 05.04, Alexander Holler wrote: >>>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy: >>>>> On 05.04, Patrick McHardy wrote: >>>> >>>>>> Basically this involves splitting the expression types into lhs >>>>>> (non-const) >>>>>> and rhs (const) parts. Keywords on the RHS side can be caught >>>>>> using an >>>>>> error statement and deferred to resolution during runtime. >>>> >>>> Sounds like trial and error. ;) >>> >>> The approach is, the patch isn't, it changes the grammar to have >>> these kinds of errors in a defined state. The patch I sent >>> however is, but I'm quite sure i understand the implications. >> >> Just to mention it, there is still the possibility to define and use >> keywords for all the icmp type names. > > Preferable with names as used in icmp.h and icmpv6.h. As these are > defines in C-headers, there is very high probability that these names > are unique, even across a large number of different sets of type or > other names. That would also remove the need to look up what name nft uses if would be clear that the names are the same as defined in c-headers. E.g. the ICMPv6 parameter-problem is good example. In the linux headers it is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations it is often named as parameter-problem. So if nft would use icmpv6_paramprob, the documentation could just refer to the c-headers. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06.04, Arturo Borrero Gonzalez wrote: > On 5 April 2015 at 14:11, Patrick McHardy <kaber@trash.net> wrote: > > > > This needs a lot of testing though since it has the potential to break > > things quite badly. Since I'm busy, maybe someone else wants to start by > > running the testsuite with this patch applied. > > I was curious and ran the testsuite with your patch. It's OK. Thanks Arturo! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06.04, Alexander Holler wrote: > Am 06.04.2015 um 11:01 schrieb Alexander Holler: > >Am 06.04.2015 um 10:44 schrieb Alexander Holler: > >>Am 06.04.2015 um 03:51 schrieb Patrick McHardy: > >>>On 05.04, Alexander Holler wrote: > >>>>Am 05.04.2015 um 14:11 schrieb Patrick McHardy: > >>>>>On 05.04, Patrick McHardy wrote: > >>>> > >>>>>>Basically this involves splitting the expression types into lhs > >>>>>>(non-const) > >>>>>>and rhs (const) parts. Keywords on the RHS side can be caught > >>>>>>using an > >>>>>>error statement and deferred to resolution during runtime. > >>>> > >>>>Sounds like trial and error. ;) > >>> > >>>The approach is, the patch isn't, it changes the grammar to have > >>>these kinds of errors in a defined state. The patch I sent > >>>however is, but I'm quite sure i understand the implications. > >> > >>Just to mention it, there is still the possibility to define and use > >>keywords for all the icmp type names. > > > >Preferable with names as used in icmp.h and icmpv6.h. As these are > >defines in C-headers, there is very high probability that these names > >are unique, even across a large number of different sets of type or > >other names. > > That would also remove the need to look up what name nft uses if would be > clear that the names are the same as defined in c-headers. > > E.g. the ICMPv6 parameter-problem is good example. In the linux headers it > is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations > it is often named as parameter-problem. > > So if nft would use icmpv6_paramprob, the documentation could just refer to > the c-headers. No, an icmpv6_ prefix is redundant and I don't see any benefit in doing that. "Documentations", whatever that is, don't matter, what matters is our documentation. And whether we point people to that or some header file really doesn't matter, except that we don't expect people to read header files to use nft. Its a tool for admins, not programmers. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.04.2015 um 13:25 schrieb Patrick McHardy: > On 06.04, Alexander Holler wrote: >> Am 06.04.2015 um 11:01 schrieb Alexander Holler: >>> Am 06.04.2015 um 10:44 schrieb Alexander Holler: >>>> Am 06.04.2015 um 03:51 schrieb Patrick McHardy: >>>>> On 05.04, Alexander Holler wrote: >>>>>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy: >>>>>>> On 05.04, Patrick McHardy wrote: >>>>>> >>>>>>>> Basically this involves splitting the expression types into lhs >>>>>>>> (non-const) >>>>>>>> and rhs (const) parts. Keywords on the RHS side can be caught >>>>>>>> using an >>>>>>>> error statement and deferred to resolution during runtime. >>>>>> >>>>>> Sounds like trial and error. ;) >>>>> >>>>> The approach is, the patch isn't, it changes the grammar to have >>>>> these kinds of errors in a defined state. The patch I sent >>>>> however is, but I'm quite sure i understand the implications. >>>> >>>> Just to mention it, there is still the possibility to define and use >>>> keywords for all the icmp type names. >>> >>> Preferable with names as used in icmp.h and icmpv6.h. As these are >>> defines in C-headers, there is very high probability that these names >>> are unique, even across a large number of different sets of type or >>> other names. >> >> That would also remove the need to look up what name nft uses if would be >> clear that the names are the same as defined in c-headers. >> >> E.g. the ICMPv6 parameter-problem is good example. In the linux headers it >> is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations >> it is often named as parameter-problem. >> >> So if nft would use icmpv6_paramprob, the documentation could just refer to >> the c-headers. > > No, an icmpv6_ prefix is redundant and I don't see any benefit in doing > that. "Documentations", whatever that is, don't matter, what matters is > our documentation. And whether we point people to that or some header > file really doesn't matter, except that we don't expect people to read > header files to use nft. Its a tool for admins, not programmers. Obviously it isn't redundant, otherwise I wouldn't had to write a patch for a problem which now exists since quiet some time (can't remember when I've first stumbled over it, maybe half a year or even a year). Anyway, thanks for fixing it, regardless if you apply my patch or something else. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.04.2015 um 13:25 schrieb Patrick McHardy: > On 06.04, Alexander Holler wrote: >> Am 06.04.2015 um 11:01 schrieb Alexander Holler: >>> Am 06.04.2015 um 10:44 schrieb Alexander Holler: >>>> Am 06.04.2015 um 03:51 schrieb Patrick McHardy: >>>>> On 05.04, Alexander Holler wrote: >>>>>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy: >>>>>>> On 05.04, Patrick McHardy wrote: >>>>>> >>>>>>>> Basically this involves splitting the expression types into lhs >>>>>>>> (non-const) >>>>>>>> and rhs (const) parts. Keywords on the RHS side can be caught >>>>>>>> using an >>>>>>>> error statement and deferred to resolution during runtime. >>>>>> >>>>>> Sounds like trial and error. ;) >>>>> >>>>> The approach is, the patch isn't, it changes the grammar to have >>>>> these kinds of errors in a defined state. The patch I sent >>>>> however is, but I'm quite sure i understand the implications. >>>> >>>> Just to mention it, there is still the possibility to define and use >>>> keywords for all the icmp type names. >>> >>> Preferable with names as used in icmp.h and icmpv6.h. As these are >>> defines in C-headers, there is very high probability that these names >>> are unique, even across a large number of different sets of type or >>> other names. >> >> That would also remove the need to look up what name nft uses if would be >> clear that the names are the same as defined in c-headers. >> >> E.g. the ICMPv6 parameter-problem is good example. In the linux headers it >> is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations >> it is often named as parameter-problem. >> >> So if nft would use icmpv6_paramprob, the documentation could just refer to >> the c-headers. > > No, an icmpv6_ prefix is redundant and I don't see any benefit in doing > that. "Documentations", whatever that is, don't matter, what matters is > our documentation. And whether we point people to that or some header > file really doesn't matter, except that we don't expect people to read > header files to use nft. Its a tool for admins, not programmers. Even some admins can code. Because nft is just at 0.4 and not widely used (or usable), I decided it's worse to write another follow up in order to try to fix things before they can't be fixed anymore. 1. I don't think that a brutforce-approach like "if error try something else" is the right way to fix a problem in the parser. 2. "icmpv6_paramprob" (or even something like "icmpv6_parameter-problem") doesn't have something redundant. It's a name for constant and e.g. "icmp_redirect" is a much more descriptive name than just "redirect" because "icmp_redirect" describes the context too and thus the name is much more verbose and readable (besides that it would fix the problem the parser currently has). 3. I don't see why admins have to use another set of names for constants than developers. Inventing a new set of names for a list of constants for which there already exist a very widely used set of names just leads to more confusion. And if it's ok to invent new names, why does nft use "param-problem" and not "parameter-problem"? Of course, I would suggest to use the existing name icmp_parameterprob (like it's used in every c/c++-source). Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09.04, Alexander Holler wrote: > >>E.g. the ICMPv6 parameter-problem is good example. In the linux headers it > >>is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations > >>it is often named as parameter-problem. > >> > >>So if nft would use icmpv6_paramprob, the documentation could just refer to > >>the c-headers. > > > >No, an icmpv6_ prefix is redundant and I don't see any benefit in doing > >that. "Documentations", whatever that is, don't matter, what matters is > >our documentation. And whether we point people to that or some header > >file really doesn't matter, except that we don't expect people to read > >header files to use nft. Its a tool for admins, not programmers. > > Even some admins can code. > > Because nft is just at 0.4 and not widely used (or usable), I decided it's > worse to write another follow up in order to try to fix things before they > can't be fixed anymore. > > 1. I don't think that a brutforce-approach like "if error try something > else" is the right way to fix a problem in the parser. Its perfectly fine in this case since the error is only caught in spots where we expect symbolic constants. So basically its treating the next keyword as symbolic constant no matter what. > 2. "icmpv6_paramprob" (or even something like "icmpv6_parameter-problem") > doesn't have something redundant. It's a name for constant and e.g. > "icmp_redirect" is a much more descriptive name than just "redirect" because > "icmp_redirect" describes the context too and thus the name is much more > verbose and readable (besides that it would fix the problem the parser > currently has). It is redundant since it can only occur in the context of ICMP expressions. icmp type icmp_redirect is obviously redundant. > 3. I don't see why admins have to use another set of names for constants > than developers. Inventing a new set of names for a list of constants for > which there already exist a very widely used set of names just leads to more > confusion. And if it's ok to invent new names, why does nft use > "param-problem" and not "parameter-problem"? Of course, I would suggest to > use the existing name icmp_parameterprob (like it's used in every > c/c++-source). In case of ICMP we use the same names that iptables used, so this actually spares admins from getting used to new constants. We're not going to use source code identifiers, there's no benefit at all, especially if you consider that Linux headers use different identifiers than the BSD headers. But I agree, ICMPv6 shouldn't use param-problem but parameter-problem for consistency with both ICMP and ip6tables. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 09.04.2015 um 13:07 schrieb Patrick McHardy: > On 09.04, Alexander Holler wrote: >>>> E.g. the ICMPv6 parameter-problem is good example. In the linux headers it >>>> is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations >>>> it is often named as parameter-problem. >>>> >>>> So if nft would use icmpv6_paramprob, the documentation could just refer to >>>> the c-headers. >>> >>> No, an icmpv6_ prefix is redundant and I don't see any benefit in doing >>> that. "Documentations", whatever that is, don't matter, what matters is >>> our documentation. And whether we point people to that or some header >>> file really doesn't matter, except that we don't expect people to read >>> header files to use nft. Its a tool for admins, not programmers. >> >> Even some admins can code. >> >> Because nft is just at 0.4 and not widely used (or usable), I decided it's >> worse to write another follow up in order to try to fix things before they >> can't be fixed anymore. >> >> 1. I don't think that a brutforce-approach like "if error try something >> else" is the right way to fix a problem in the parser. > > Its perfectly fine in this case since the error is only caught in spots > where we expect symbolic constants. So basically its treating the next > keyword as symbolic constant no matter what. I wonder how the error routine in that patch knowns that a symbolic constant is expected. Where does it get that information from? And if so, why is in such a case error called anyways? And how are problems in the grammar identified in which a token or string might match? Of course, my bison knowledge got very rusty as I haven't fiddled with it for a long time, but I still think it's brute-force or trial-and-error approach. >> 2. "icmpv6_paramprob" (or even something like "icmpv6_parameter-problem") >> doesn't have something redundant. It's a name for constant and e.g. >> "icmp_redirect" is a much more descriptive name than just "redirect" because >> "icmp_redirect" describes the context too and thus the name is much more >> verbose and readable (besides that it would fix the problem the parser >> currently has). > > It is redundant since it can only occur in the context of ICMP expressions. > icmp type icmp_redirect is obviously redundant. It's just a name as such it can't be redundant if you want to allow names. Of course, there might be people which are unaware that "icmp_" in "icmp_redirect" is just part of the name for a constant, but those people shouldn't write firewall rules anyway. >> 3. I don't see why admins have to use another set of names for constants >> than developers. Inventing a new set of names for a list of constants for >> which there already exist a very widely used set of names just leads to more >> confusion. And if it's ok to invent new names, why does nft use >> "param-problem" and not "parameter-problem"? Of course, I would suggest to >> use the existing name icmp_parameterprob (like it's used in every >> c/c++-source). > > In case of ICMP we use the same names that iptables used, so this actually > spares admins from getting used to new constants. We're not going to use > source code identifiers, there's no benefit at all, especially if you > consider that Linux headers use different identifiers than the BSD headers. nft isn't in use on BSD and if you think taking BSD out of a corner makes sense, I wonder how compatible the names, nft uses, are, with what is used by ipf or one of the other BSD firewall packages. As the answer is the names are incompatible, arguing with BSD is nonsense here. > > But I agree, ICMPv6 shouldn't use param-problem but parameter-problem for > consistency with both ICMP and ip6tables. Anyway, I've tried it a second time. Can't do more. So I will now entering the popcorn-state. Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09.04, Alexander Holler wrote: > Am 09.04.2015 um 13:07 schrieb Patrick McHardy: > >>3. I don't see why admins have to use another set of names for constants > >>than developers. Inventing a new set of names for a list of constants for > >>which there already exist a very widely used set of names just leads to more > >>confusion. And if it's ok to invent new names, why does nft use > >>"param-problem" and not "parameter-problem"? Of course, I would suggest to > >>use the existing name icmp_parameterprob (like it's used in every > >>c/c++-source). > > > >In case of ICMP we use the same names that iptables used, so this actually > >spares admins from getting used to new constants. We're not going to use > >source code identifiers, there's no benefit at all, especially if you > >consider that Linux headers use different identifiers than the BSD headers. > > nft isn't in use on BSD and if you think taking BSD out of a corner makes > sense, I wonder how compatible the names, nft uses, are, with what is used > by ipf or one of the other BSD firewall packages. As the answer is the names > are incompatible, arguing with BSD is nonsense here. This is starting to annoy me. If you suggest to use names from headers, at least do your homework. The BSD headers is what most of userspace uses, and this is where ICMP_PARAMPROB originates. Linux uses ICMP_PARAMETERPROB. > >But I agree, ICMPv6 shouldn't use param-problem but parameter-problem for > >consistency with both ICMP and ip6tables. > > Anyway, I've tried it a second time. Can't do more. So I will now entering > the popcorn-state. The decision has been made a long time ago. You're wasting both our time. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 09.04.2015 um 21:15 schrieb Patrick McHardy: > On 09.04, Alexander Holler wrote: >> Am 09.04.2015 um 13:07 schrieb Patrick McHardy: >>>> 3. I don't see why admins have to use another set of names for constants >>>> than developers. Inventing a new set of names for a list of constants for >>>> which there already exist a very widely used set of names just leads to more >>>> confusion. And if it's ok to invent new names, why does nft use >>>> "param-problem" and not "parameter-problem"? Of course, I would suggest to >>>> use the existing name icmp_parameterprob (like it's used in every >>>> c/c++-source). >>> >>> In case of ICMP we use the same names that iptables used, so this actually >>> spares admins from getting used to new constants. We're not going to use >>> source code identifiers, there's no benefit at all, especially if you >>> consider that Linux headers use different identifiers than the BSD headers. >> >> nft isn't in use on BSD and if you think taking BSD out of a corner makes >> sense, I wonder how compatible the names, nft uses, are, with what is used >> by ipf or one of the other BSD firewall packages. As the answer is the names >> are incompatible, arguing with BSD is nonsense here. > > This is starting to annoy me. If you suggest to use names from headers, at > least do your homework. The BSD headers is what most of userspace uses, and > this is where ICMP_PARAMPROB originates. Linux uses ICMP_PARAMETERPROB. I aggree that this discussion, which never really has begun, finally ended in the mud. Feel free to throw out more accusations as I won't try to discuss with you anymore. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/parser_bison.y b/src/parser_bison.y index b86381d..8d39c67 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -1583,6 +1583,30 @@ symbol_expr : string $2); xfree($2); } + | error + { + struct error_record *erec; + char *tmp; + + if (yytoken != TOKEN_EOF) { + tmp = xstrdup(yytname[yytoken] + 1); + tmp[strlen(tmp) - 1] = '\0'; + $$ = symbol_expr_alloc(&@$, SYMBOL_VALUE, + current_scope(state), + tmp); + xfree(tmp); + + erec = list_entry(state->msgs->prev, + struct error_record, list); + list_del(&erec->list); + xfree(erec); + + yyclearin; + yyerrok; + } else { + YYABORT; + } + } ; integer_expr : NUM