| Message ID | 1492704342-24042-5-git-send-email-pme.lebleu@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Inline… > On Apr 20, 2017, at 10:05 AM, Pierre Lebleu <pme.lebleu@gmail.com> wrote: > > It gives the ability to create ipset rules via procd > services and netifd interface firewall data. > > Signed-off-by: Pierre Lebleu <pme.lebleu@gmail.com> > --- > ipsets.c | 83 +++++++++++++++++++++++++++++++++++++++++++------------------- > ipsets.h | 11 +++++---- > main.c | 2 +- > 3 files changed, 65 insertions(+), 31 deletions(-) > > diff --git a/ipsets.c b/ipsets.c > index 3b1ba00..a3e8ee7 100644 > --- a/ipsets.c > +++ b/ipsets.c > @@ -85,7 +85,7 @@ static struct ipset_type ipset_types[] = { > > > static bool > -check_types(struct uci_element *e, struct fw3_ipset *ipset) > +check_types(struct fw3_ipset *ipset) > { > int i = 0; > uint32_t typelist = 0; > @@ -95,7 +95,8 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) > { > if (i >= 3) > { > - warn_elem(e, "must not have more than 3 datatypes assigned"); > + warn("%s must not have more than 3 datatypes assigned", > + ipset->name); > return false; > } > > @@ -116,8 +117,9 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) > { > ipset->method = ipset_types[i].method; > > - warn_elem(e, "defines no storage method, assuming '%s'", > - fw3_ipset_method_names[ipset->method]); > + warn("%s defines no storage method, assuming '%s'", > + ipset->name, > + fw3_ipset_method_names[ipset->method]); > > break; > } > @@ -136,56 +138,56 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) > if ((ipset_types[i].required & OPT_IPRANGE) && > !ipset->iprange.set) > { > - warn_elem(e, "requires an ip range"); > + warn("%s requires an ip range", ipset->name); > return false; > } > > if ((ipset_types[i].required & OPT_PORTRANGE) && > !ipset->portrange.set) > { > - warn_elem(e, "requires a port range"); > + warn("%s requires a port range", ipset->name); > return false; > } > > if (!(ipset_types[i].required & OPT_IPRANGE) && > ipset->iprange.set) > { > - warn_elem(e, "iprange ignored"); > + warn("%s iprange ignored", ipset->name); > ipset->iprange.set = false; > } > > if (!(ipset_types[i].required & OPT_PORTRANGE) && > ipset->portrange.set) > { > - warn_elem(e, "portrange ignored"); > + warn("%s portrange ignored", ipset->name); > ipset->portrange.set = false; > } > > if (!(ipset_types[i].optional & OPT_NETMASK) && > ipset->netmask > 0) > { > - warn_elem(e, "netmask ignored"); > + warn("%s netmask ignored", ipset->name); > ipset->netmask = 0; > } > > if (!(ipset_types[i].optional & OPT_HASHSIZE) && > ipset->hashsize > 0) > { > - warn_elem(e, "hashsize ignored"); > + warn("%s hashsize ignored", ipset->name); > ipset->hashsize = 0; > } > > if (!(ipset_types[i].optional & OPT_MAXELEM) && > ipset->maxelem > 0) > { > - warn_elem(e, "maxelem ignored"); > + warn("%s maxelem ignored", ipset->name); > ipset->maxelem = 0; > } > > if (!(ipset_types[i].optional & OPT_FAMILY) && > ipset->family != FW3_FAMILY_V4) > { > - warn_elem(e, "family ignored"); > + warn("%s family ignored", ipset->name); > ipset->family = FW3_FAMILY_V4; > } > } > @@ -194,12 +196,12 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) > } > } > > - warn_elem(e, "has an invalid combination of storage method and matches"); > + warn("%s has an invalid combination of storage method and matches", ipset->name); > return false; > } > > -struct fw3_ipset * > -fw3_alloc_ipset(void) > +static struct fw3_ipset * > +fw3_alloc_ipset(struct fw3_state *state) > { > struct fw3_ipset *ipset; > > @@ -212,21 +214,50 @@ fw3_alloc_ipset(void) > ipset->enabled = true; > ipset->family = FW3_FAMILY_V4; > > + list_add_tail(&ipset->list, &state->ipsets); > + > return ipset; > } > > void > -fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) > +fw3_load_ipsets(struct fw3_state *state, struct uci_package *p, > + struct blob_attr *a) > { > struct uci_section *s; > struct uci_element *e; > - struct fw3_ipset *ipset; > + struct fw3_ipset *ipset, *n; > + struct blob_attr *entry, *opt; > + unsigned rem, orem; > > INIT_LIST_HEAD(&state->ipsets); > > if (state->disable_ipsets) > return; > > + blob_for_each_attr(entry, a, rem) > + { > + const char *type = NULL; > + const char *name = "ubus ipset"; > + blobmsg_for_each_attr(opt, entry, orem) > + if (!strcmp(blobmsg_name(opt), "type")) > + type = blobmsg_get_string(opt); > + else if (!strcmp(blobmsg_name(opt), "name")) > + name = blobmsg_get_string(opt); > + > + if (!type || strcmp(type, "ipset")) > + continue; > + > + if (!(ipset = fw3_alloc_ipset(state))) Minor nit… Assignments inside of conditionals are a bear to step through in a debugger like GDB. -Philip > + continue; > + > + if (!fw3_parse_blob_options(ipset, fw3_ipset_opts, entry, name)) > + { > + warn("%s skipped due to invalid options\n", name); > + fw3_free_ipset(ipset); > + continue; > + } > + } > + > uci_foreach_element(&p->sections, e) > { > s = uci_to_section(e); > @@ -234,7 +265,7 @@ fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) > if (strcmp(s->type, "ipset")) > continue; > > - ipset = fw3_alloc_ipset(); > + ipset = fw3_alloc_ipset(state); > > if (!ipset) > continue; > @@ -245,7 +276,10 @@ fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) > fw3_free_ipset(ipset); > continue; > } > + } > > + list_for_each_entry_safe(ipset, n, &state->ipsets, list) > + { > if (ipset->external) > { > if (!*ipset->external) > @@ -256,27 +290,26 @@ fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) > > if (!ipset->name || !*ipset->name) > { > - warn_elem(e, "must have a name assigned"); > + warn("ipset must have a name assigned"); > } > //else if (fw3_lookup_ipset(state, ipset->name) != NULL) > //{ > - // warn_elem(e, "has duplicated set name '%s'", ipset->name); > + // warn("%s has duplicated set name", ipset->name); > //} > else if (ipset->family == FW3_FAMILY_ANY) > { > - warn_elem(e, "must not have family 'any'"); > + warn("%s must not have family 'any'", ipset->name); > } > else if (ipset->iprange.set && ipset->family != ipset->iprange.family) > { > - warn_elem(e, "has iprange of wrong address family"); > + warn("%s has iprange of wrong address family", ipset->name); > } > else if (list_empty(&ipset->datatypes)) > { > - warn_elem(e, "has no datatypes assigned"); > + warn("%s has no datatypes assigned", ipset->name); > } > - else if (check_types(e, ipset)) > + else if (check_types(ipset)) > { > - list_add_tail(&ipset->list, &state->ipsets); > continue; > } > > diff --git a/ipsets.h b/ipsets.h > index b5fee6c..2ba862d 100644 > --- a/ipsets.h > +++ b/ipsets.h > @@ -27,8 +27,7 @@ > > extern const struct fw3_option fw3_ipset_opts[]; > > -struct fw3_ipset * fw3_alloc_ipset(void); > -void fw3_load_ipsets(struct fw3_state *state, struct uci_package *p); > +void fw3_load_ipsets(struct fw3_state *state, struct uci_package *p, struct blob_attr *a); > void fw3_create_ipsets(struct fw3_state *state); > void fw3_destroy_ipsets(struct fw3_state *state); > > @@ -36,9 +35,11 @@ struct fw3_ipset * fw3_lookup_ipset(struct fw3_state *state, const char *name); > > bool fw3_check_ipset(struct fw3_ipset *set); > > -#define fw3_free_ipset(ipset) \ > - fw3_free_object(ipset, fw3_ipset_opts) > - > +static inline void fw3_free_ipset(struct fw3_ipset *ipset) > +{ > + list_del(&ipset->list); > + fw3_free_object(ipset, fw3_ipset_opts); > +} > > #ifndef SO_IP_SET > > diff --git a/main.c b/main.c > index 4cf46fd..6e275ef 100644 > --- a/main.c > +++ b/main.c > @@ -101,7 +101,7 @@ build_state(bool runtime) > fw3_ubus_rules(&b); > > fw3_load_defaults(state, p); > - fw3_load_ipsets(state, p); > + fw3_load_ipsets(state, p, b.head); > fw3_load_zones(state, p); > fw3_load_rules(state, p, b.head); > fw3_load_redirects(state, p, b.head); > -- > 1.7.9.5 > > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
> On May 2, 2017, at 6:15 AM, Pierre Lebleu <pme.lebleu@gmail.com> wrote: > > Hi Philip, > > 2017-04-29 3:11 GMT+02:00 Philip Prindeville <philipp_subx@redfish-solutions.com>: > Inline… > > > [snip] > > + if (!(ipset = fw3_alloc_ipset(state))) > > > Minor nit… Assignments inside of conditionals are a bear to step through in a debugger like GDB. > > -Philip > > It is a trivial assignment and it is already done in this style along the file. > > -- > Pierre > It’s not about trivial vs. nontrivial. It’s about whether you could step through the assignment with (say) gdb, execute just the assignment, examine the value, and then step through the “if”. And the answer is, “you can’t”. Because gdb is a source level debugger where the unit of source is the “line”. (Actually, it’s also the unit of source for gcc when it generates debugging symbols.) The way to separate to 2 individual statements in C (for the purposes of gdb debugging) is to put them on separate lines. Yes, that’s a glaring limitation of gcc and gdb, but that’s our reality. As for what’s already done in this style in the file… Having separate assignments and tests is *also* done, and indeed it’s done more often. -Philip
Assignment within a condition is easily read by (dyslectic) humans as a test for equality (==) and is for that reason als better avoided. Paul > Op 2 mei 2017, om 18:43 heeft Philip Prindeville <philipp_subx@redfish-solutions.com> het volgende geschreven: > > >> On May 2, 2017, at 6:15 AM, Pierre Lebleu <pme.lebleu@gmail.com> wrote: >> >> Hi Philip, >> >> 2017-04-29 3:11 GMT+02:00 Philip Prindeville <philipp_subx@redfish-solutions.com>: >> Inline… >> >> >> [snip] >>> + if (!(ipset = fw3_alloc_ipset(state))) >> >> >> Minor nit… Assignments inside of conditionals are a bear to step through in a debugger like GDB. >> >> -Philip >> >> It is a trivial assignment and it is already done in this style along the file. >> >> -- >> Pierre >> > > > It’s not about trivial vs. nontrivial. It’s about whether you could step through the assignment with (say) gdb, execute just the assignment, examine the value, and then step through the “if”. And the answer is, “you can’t”. Because gdb is a source level debugger where the unit of source is the “line”. > > (Actually, it’s also the unit of source for gcc when it generates debugging symbols.) > > The way to separate to 2 individual statements in C (for the purposes of gdb debugging) is to put them on separate lines. Yes, that’s a glaring limitation of gcc and gdb, but that’s our reality. > > As for what’s already done in this style in the file… Having separate assignments and tests is *also* done, and indeed it’s done more often. > > -Philip > > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
On 05/02/2017 05:36 PM, Paul Oranje wrote: > Assignment within a condition is easily read by (dyslectic) humans as a test for equality (==) and is for that reason als better avoided. > Paul > >> Op 2 mei 2017, om 18:43 heeft Philip Prindeville <philipp_subx@redfish-solutions.com> het volgende geschreven: >> >> >>> On May 2, 2017, at 6:15 AM, Pierre Lebleu <pme.lebleu@gmail.com> wrote: >>> >>> Hi Philip, >>> >>> 2017-04-29 3:11 GMT+02:00 Philip Prindeville <philipp_subx@redfish-solutions.com>: >>> Inline… >>> >>> >>> [snip] >>>> + if (!(ipset = fw3_alloc_ipset(state))) >>> >>> >>> Minor nit… Assignments inside of conditionals are a bear to step through in a debugger like GDB. >>> >>> -Philip >>> >>> It is a trivial assignment and it is already done in this style along the file. >>> >>> -- >>> Pierre >>> >> >> >> It’s not about trivial vs. nontrivial. It’s about whether you could step through the assignment with (say) gdb, execute just the assignment, examine the value, and then step through the “if”. And the answer is, “you can’t”. Because gdb is a source level debugger where the unit of source is the “line”. >> >> (Actually, it’s also the unit of source for gcc when it generates debugging symbols.) >> >> The way to separate to 2 individual statements in C (for the purposes of gdb debugging) is to put them on separate lines. Yes, that’s a glaring limitation of gcc and gdb, but that’s our reality. >> >> As for what’s already done in this style in the file… Having separate assignments and tests is *also* done, and indeed it’s done more often. >> >> -Philip Others may have been clever but it doesn't mean a construct should continue. This if-assignment combo and other clever C constructs are frowned upon in popular software quality standards. Many simply are due to the above mentioned maintainability issues. Some industries may be more sensitive, but the rationale for avoidance is solid.
2017-05-03 3:54 GMT+02:00 Eric Luehrsen <ericluehrsen@hotmail.com>: > > > On 05/02/2017 05:36 PM, Paul Oranje wrote: >> Assignment within a condition is easily read by (dyslectic) humans as a test for equality (==) and is for that reason als better avoided. >> Paul >> >>> Op 2 mei 2017, om 18:43 heeft Philip Prindeville <philipp_subx@redfish-solutions.com> het volgende geschreven: >>> >>> >>>> On May 2, 2017, at 6:15 AM, Pierre Lebleu <pme.lebleu@gmail.com> wrote: >>>> >>>> Hi Philip, >>>> >>>> 2017-04-29 3:11 GMT+02:00 Philip Prindeville <philipp_subx@redfish-solutions.com>: >>>> Inline… >>>> >>>> >>>> [snip] >>>>> + if (!(ipset = fw3_alloc_ipset(state))) >>>> >>>> >>>> Minor nit… Assignments inside of conditionals are a bear to step through in a debugger like GDB. >>>> >>>> -Philip >>>> >>>> It is a trivial assignment and it is already done in this style along the file. >>>> >>>> -- >>>> Pierre >>>> >>> >>> >>> It’s not about trivial vs. nontrivial. It’s about whether you could step through the assignment with (say) gdb, execute just the assignment, examine the value, and then step through the “if”. And the answer is, “you can’t”. Because gdb is a source level debugger where the unit of source is the “line”. >>> >>> (Actually, it’s also the unit of source for gcc when it generates debugging symbols.) >>> >>> The way to separate to 2 individual statements in C (for the purposes of gdb debugging) is to put them on separate lines. Yes, that’s a glaring limitation of gcc and gdb, but that’s our reality. >>> >>> As for what’s already done in this style in the file… Having separate assignments and tests is *also* done, and indeed it’s done more often. >>> >>> -Philip > > Others may have been clever but it doesn't mean a construct should > continue. This if-assignment combo and other clever C constructs are > frowned upon in popular software quality standards. Many simply are due > to the above mentioned maintainability issues. Some industries may be > more sensitive, but the rationale for avoidance is solid. > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev Ok, I take your remark and comply with it.
diff --git a/ipsets.c b/ipsets.c index 3b1ba00..a3e8ee7 100644 --- a/ipsets.c +++ b/ipsets.c @@ -85,7 +85,7 @@ static struct ipset_type ipset_types[] = { static bool -check_types(struct uci_element *e, struct fw3_ipset *ipset) +check_types(struct fw3_ipset *ipset) { int i = 0; uint32_t typelist = 0; @@ -95,7 +95,8 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) { if (i >= 3) { - warn_elem(e, "must not have more than 3 datatypes assigned"); + warn("%s must not have more than 3 datatypes assigned", + ipset->name); return false; } @@ -116,8 +117,9 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) { ipset->method = ipset_types[i].method; - warn_elem(e, "defines no storage method, assuming '%s'", - fw3_ipset_method_names[ipset->method]); + warn("%s defines no storage method, assuming '%s'", + ipset->name, + fw3_ipset_method_names[ipset->method]); break; } @@ -136,56 +138,56 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) if ((ipset_types[i].required & OPT_IPRANGE) && !ipset->iprange.set) { - warn_elem(e, "requires an ip range"); + warn("%s requires an ip range", ipset->name); return false; } if ((ipset_types[i].required & OPT_PORTRANGE) && !ipset->portrange.set) { - warn_elem(e, "requires a port range"); + warn("%s requires a port range", ipset->name); return false; } if (!(ipset_types[i].required & OPT_IPRANGE) && ipset->iprange.set) { - warn_elem(e, "iprange ignored"); + warn("%s iprange ignored", ipset->name); ipset->iprange.set = false; } if (!(ipset_types[i].required & OPT_PORTRANGE) && ipset->portrange.set) { - warn_elem(e, "portrange ignored"); + warn("%s portrange ignored", ipset->name); ipset->portrange.set = false; } if (!(ipset_types[i].optional & OPT_NETMASK) && ipset->netmask > 0) { - warn_elem(e, "netmask ignored"); + warn("%s netmask ignored", ipset->name); ipset->netmask = 0; } if (!(ipset_types[i].optional & OPT_HASHSIZE) && ipset->hashsize > 0) { - warn_elem(e, "hashsize ignored"); + warn("%s hashsize ignored", ipset->name); ipset->hashsize = 0; } if (!(ipset_types[i].optional & OPT_MAXELEM) && ipset->maxelem > 0) { - warn_elem(e, "maxelem ignored"); + warn("%s maxelem ignored", ipset->name); ipset->maxelem = 0; } if (!(ipset_types[i].optional & OPT_FAMILY) && ipset->family != FW3_FAMILY_V4) { - warn_elem(e, "family ignored"); + warn("%s family ignored", ipset->name); ipset->family = FW3_FAMILY_V4; } } @@ -194,12 +196,12 @@ check_types(struct uci_element *e, struct fw3_ipset *ipset) } } - warn_elem(e, "has an invalid combination of storage method and matches"); + warn("%s has an invalid combination of storage method and matches", ipset->name); return false; } -struct fw3_ipset * -fw3_alloc_ipset(void) +static struct fw3_ipset * +fw3_alloc_ipset(struct fw3_state *state) { struct fw3_ipset *ipset; @@ -212,21 +214,50 @@ fw3_alloc_ipset(void) ipset->enabled = true; ipset->family = FW3_FAMILY_V4; + list_add_tail(&ipset->list, &state->ipsets); + return ipset; } void -fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) +fw3_load_ipsets(struct fw3_state *state, struct uci_package *p, + struct blob_attr *a) { struct uci_section *s; struct uci_element *e; - struct fw3_ipset *ipset; + struct fw3_ipset *ipset, *n; + struct blob_attr *entry, *opt; + unsigned rem, orem; INIT_LIST_HEAD(&state->ipsets); if (state->disable_ipsets) return; + blob_for_each_attr(entry, a, rem) + { + const char *type = NULL; + const char *name = "ubus ipset"; + blobmsg_for_each_attr(opt, entry, orem) + if (!strcmp(blobmsg_name(opt), "type")) + type = blobmsg_get_string(opt); + else if (!strcmp(blobmsg_name(opt), "name")) + name = blobmsg_get_string(opt); + + if (!type || strcmp(type, "ipset")) + continue; + + if (!(ipset = fw3_alloc_ipset(state))) + continue; + + if (!fw3_parse_blob_options(ipset, fw3_ipset_opts, entry, name)) + { + warn("%s skipped due to invalid options\n", name); + fw3_free_ipset(ipset); + continue; + } + } + uci_foreach_element(&p->sections, e) { s = uci_to_section(e); @@ -234,7 +265,7 @@ fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) if (strcmp(s->type, "ipset")) continue; - ipset = fw3_alloc_ipset(); + ipset = fw3_alloc_ipset(state); if (!ipset) continue; @@ -245,7 +276,10 @@ fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) fw3_free_ipset(ipset); continue; } + } + list_for_each_entry_safe(ipset, n, &state->ipsets, list) + { if (ipset->external) { if (!*ipset->external) @@ -256,27 +290,26 @@ fw3_load_ipsets(struct fw3_state *state, struct uci_package *p) if (!ipset->name || !*ipset->name) { - warn_elem(e, "must have a name assigned"); + warn("ipset must have a name assigned"); } //else if (fw3_lookup_ipset(state, ipset->name) != NULL) //{ - // warn_elem(e, "has duplicated set name '%s'", ipset->name); + // warn("%s has duplicated set name", ipset->name); //} else if (ipset->family == FW3_FAMILY_ANY) { - warn_elem(e, "must not have family 'any'"); + warn("%s must not have family 'any'", ipset->name); } else if (ipset->iprange.set && ipset->family != ipset->iprange.family) { - warn_elem(e, "has iprange of wrong address family"); + warn("%s has iprange of wrong address family", ipset->name); } else if (list_empty(&ipset->datatypes)) { - warn_elem(e, "has no datatypes assigned"); + warn("%s has no datatypes assigned", ipset->name); } - else if (check_types(e, ipset)) + else if (check_types(ipset)) { - list_add_tail(&ipset->list, &state->ipsets); continue; } diff --git a/ipsets.h b/ipsets.h index b5fee6c..2ba862d 100644 --- a/ipsets.h +++ b/ipsets.h @@ -27,8 +27,7 @@ extern const struct fw3_option fw3_ipset_opts[]; -struct fw3_ipset * fw3_alloc_ipset(void); -void fw3_load_ipsets(struct fw3_state *state, struct uci_package *p); +void fw3_load_ipsets(struct fw3_state *state, struct uci_package *p, struct blob_attr *a); void fw3_create_ipsets(struct fw3_state *state); void fw3_destroy_ipsets(struct fw3_state *state); @@ -36,9 +35,11 @@ struct fw3_ipset * fw3_lookup_ipset(struct fw3_state *state, const char *name); bool fw3_check_ipset(struct fw3_ipset *set); -#define fw3_free_ipset(ipset) \ - fw3_free_object(ipset, fw3_ipset_opts) - +static inline void fw3_free_ipset(struct fw3_ipset *ipset) +{ + list_del(&ipset->list); + fw3_free_object(ipset, fw3_ipset_opts); +} #ifndef SO_IP_SET diff --git a/main.c b/main.c index 4cf46fd..6e275ef 100644 --- a/main.c +++ b/main.c @@ -101,7 +101,7 @@ build_state(bool runtime) fw3_ubus_rules(&b); fw3_load_defaults(state, p); - fw3_load_ipsets(state, p); + fw3_load_ipsets(state, p, b.head); fw3_load_zones(state, p); fw3_load_rules(state, p, b.head); fw3_load_redirects(state, p, b.head);
It gives the ability to create ipset rules via procd services and netifd interface firewall data. Signed-off-by: Pierre Lebleu <pme.lebleu@gmail.com> --- ipsets.c | 83 +++++++++++++++++++++++++++++++++++++++++++------------------- ipsets.h | 11 +++++---- main.c | 2 +- 3 files changed, 65 insertions(+), 31 deletions(-)