* [PATCH RESEND] Add element count to hash headers @ 2015-02-18 19:53 Eric B Munson 2015-03-04 3:34 ` Josh Hunt 0 siblings, 1 reply; 7+ messages in thread From: Eric B Munson @ 2015-02-18 19:53 UTC (permalink / raw) To: netfilter-devel Cc: Eric B Munson, Jozsef Kadlecsik, Pablo Neira Ayuso, Josh Hunt It would be useful for userspace to query the size of an ipset hash, however, this data is not exposed to userspace outside of counting the number of member entries. This patch uses the attribute IPSET_ATTR_ELEMENTS to indicate the size in the the header that is exported to userspace. This field is then printed by the userspace tool for hashes. Because it is only meaningful for hashes to report their size, the output is conditional on the set type. To do this checking the MATCH_TYPENAME macro was moved to utils.h. Signed-off-by: Eric B Munson <emunson@akamai.com> Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> Cc: Pablo Neira Ayuso <pablo@netfilter.org> Cc: Josh Hunt <johunt@akamai.com> --- include/libipset/utils.h | 3 +++ kernel/net/netfilter/ipset/ip_set_hash_gen.h | 3 ++- lib/errcode.c | 2 -- lib/session.c | 14 ++++++++++++-- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/include/libipset/utils.h b/include/libipset/utils.h index 3cd29da..ceedd45 100644 --- a/include/libipset/utils.h +++ b/include/libipset/utils.h @@ -19,6 +19,9 @@ #define STRCASEQ(a, b) (strcasecmp(a, b) == 0) #define STRNCASEQ(a, b, n) (strncasecmp(a, b, n) == 0) +/* Match set type names */ +#define MATCH_TYPENAME(a, b) STRNEQ(a, b, strlen(b)) + /* Stringify tokens */ #define _STR(c) #c #define STR(c) _STR(c) diff --git a/kernel/net/netfilter/ipset/ip_set_hash_gen.h b/kernel/net/netfilter/ipset/ip_set_hash_gen.h index 885105b..2000a20 100644 --- a/kernel/net/netfilter/ipset/ip_set_hash_gen.h +++ b/kernel/net/netfilter/ipset/ip_set_hash_gen.h @@ -1040,7 +1040,8 @@ mtype_head(struct ip_set *set, struct sk_buff *skb) goto nla_put_failure; #endif if (nla_put_net32(skb, IPSET_ATTR_REFERENCES, htonl(set->ref - 1)) || - nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize))) + nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize)) || + nla_put_net32(skb, IPSET_ATTR_ELEMENTS, htonl(h->elements))) goto nla_put_failure; if (unlikely(ip_set_put_flags(skb, set))) goto nla_put_failure; diff --git a/lib/errcode.c b/lib/errcode.c index 8eb275b..3881121 100644 --- a/lib/errcode.c +++ b/lib/errcode.c @@ -148,8 +148,6 @@ static const struct ipset_errcode_table list_errcode_table[] = { { }, }; -#define MATCH_TYPENAME(a, b) STRNEQ(a, b, strlen(b)) - /** * ipset_errcode - interpret a kernel error code * @session: session structure diff --git a/lib/session.c b/lib/session.c index 013d9d8..07f3396 100644 --- a/lib/session.c +++ b/lib/session.c @@ -931,6 +931,10 @@ list_create(struct ipset_session *session, struct nlattr *nla[]) safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); safe_snprintf(session, "\nReferences: "); safe_dprintf(session, ipset_print_number, IPSET_OPT_REFERENCES); + if (MATCH_TYPENAME(type->name , "hash:")) { + safe_snprintf(session, "\nNum Entries: "); + safe_dprintf(session, ipset_print_number, IPSET_OPT_ELEMENTS); + } safe_snprintf(session, session->envopts & IPSET_ENV_LIST_HEADER ? "\n" : "\nMembers:\n"); @@ -940,10 +944,16 @@ list_create(struct ipset_session *session, struct nlattr *nla[]) safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); safe_snprintf(session, "</memsize>\n<references>"); safe_dprintf(session, ipset_print_number, IPSET_OPT_REFERENCES); + safe_snprintf(session, "</references>\n"); + if (MATCH_TYPENAME(type->name , "hash:")) { + safe_snprintf(session, "<numentries>"); + safe_dprintf(session, ipset_print_number, IPSET_OPT_ELEMENTS); + safe_snprintf(session, "</numentries>\n"); + } safe_snprintf(session, session->envopts & IPSET_ENV_LIST_HEADER ? - "</references>\n</header>\n" : - "</references>\n</header>\n<members>\n"); + "</header>\n" : + "</header>\n<members>\n"); break; default: break; -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Add element count to hash headers 2015-02-18 19:53 [PATCH RESEND] Add element count to hash headers Eric B Munson @ 2015-03-04 3:34 ` Josh Hunt 2015-03-04 14:39 ` Eric B Munson 2015-03-06 21:45 ` Jozsef Kadlecsik 0 siblings, 2 replies; 7+ messages in thread From: Josh Hunt @ 2015-03-04 3:34 UTC (permalink / raw) To: Jozsef Kadlecsik; +Cc: Eric B Munson, netfilter-devel, Pablo Neira Ayuso [-- Attachment #1: Type: text/plain, Size: 5106 bytes --] On 02/18/2015 01:53 PM, Eric B Munson wrote: > It would be useful for userspace to query the size of an ipset hash, > however, this data is not exposed to userspace outside of counting the > number of member entries. This patch uses the attribute > IPSET_ATTR_ELEMENTS to indicate the size in the the header that is > exported to userspace. This field is then printed by the userspace > tool for hashes. > > Because it is only meaningful for hashes to report their size, the > output is conditional on the set type. To do this checking the > MATCH_TYPENAME macro was moved to utils.h. > > Signed-off-by: Eric B Munson <emunson@akamai.com> > Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Cc: Josh Hunt <johunt@akamai.com> > --- > include/libipset/utils.h | 3 +++ > kernel/net/netfilter/ipset/ip_set_hash_gen.h | 3 ++- > lib/errcode.c | 2 -- > lib/session.c | 14 ++++++++++++-- > 4 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/include/libipset/utils.h b/include/libipset/utils.h > index 3cd29da..ceedd45 100644 > --- a/include/libipset/utils.h > +++ b/include/libipset/utils.h > @@ -19,6 +19,9 @@ > #define STRCASEQ(a, b) (strcasecmp(a, b) == 0) > #define STRNCASEQ(a, b, n) (strncasecmp(a, b, n) == 0) > > +/* Match set type names */ > +#define MATCH_TYPENAME(a, b) STRNEQ(a, b, strlen(b)) > + > /* Stringify tokens */ > #define _STR(c) #c > #define STR(c) _STR(c) > diff --git a/kernel/net/netfilter/ipset/ip_set_hash_gen.h b/kernel/net/netfilter/ipset/ip_set_hash_gen.h > index 885105b..2000a20 100644 > --- a/kernel/net/netfilter/ipset/ip_set_hash_gen.h > +++ b/kernel/net/netfilter/ipset/ip_set_hash_gen.h > @@ -1040,7 +1040,8 @@ mtype_head(struct ip_set *set, struct sk_buff *skb) > goto nla_put_failure; > #endif > if (nla_put_net32(skb, IPSET_ATTR_REFERENCES, htonl(set->ref - 1)) || > - nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize))) > + nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize)) || > + nla_put_net32(skb, IPSET_ATTR_ELEMENTS, htonl(h->elements))) > goto nla_put_failure; > if (unlikely(ip_set_put_flags(skb, set))) > goto nla_put_failure; > diff --git a/lib/errcode.c b/lib/errcode.c > index 8eb275b..3881121 100644 > --- a/lib/errcode.c > +++ b/lib/errcode.c > @@ -148,8 +148,6 @@ static const struct ipset_errcode_table list_errcode_table[] = { > { }, > }; > > -#define MATCH_TYPENAME(a, b) STRNEQ(a, b, strlen(b)) > - > /** > * ipset_errcode - interpret a kernel error code > * @session: session structure > diff --git a/lib/session.c b/lib/session.c > index 013d9d8..07f3396 100644 > --- a/lib/session.c > +++ b/lib/session.c > @@ -931,6 +931,10 @@ list_create(struct ipset_session *session, struct nlattr *nla[]) > safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); > safe_snprintf(session, "\nReferences: "); > safe_dprintf(session, ipset_print_number, IPSET_OPT_REFERENCES); > + if (MATCH_TYPENAME(type->name , "hash:")) { > + safe_snprintf(session, "\nNum Entries: "); > + safe_dprintf(session, ipset_print_number, IPSET_OPT_ELEMENTS); > + } > safe_snprintf(session, > session->envopts & IPSET_ENV_LIST_HEADER ? > "\n" : "\nMembers:\n"); > @@ -940,10 +944,16 @@ list_create(struct ipset_session *session, struct nlattr *nla[]) > safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); > safe_snprintf(session, "</memsize>\n<references>"); > safe_dprintf(session, ipset_print_number, IPSET_OPT_REFERENCES); > + safe_snprintf(session, "</references>\n"); > + if (MATCH_TYPENAME(type->name , "hash:")) { > + safe_snprintf(session, "<numentries>"); > + safe_dprintf(session, ipset_print_number, IPSET_OPT_ELEMENTS); > + safe_snprintf(session, "</numentries>\n"); > + } > safe_snprintf(session, > session->envopts & IPSET_ENV_LIST_HEADER ? > - "</references>\n</header>\n" : > - "</references>\n</header>\n<members>\n"); > + "</header>\n" : > + "</header>\n<members>\n"); > break; > default: > break; > Jozsef Can you clarify what you're looking for when you mentioned (in a previous mail) "I don't like any change which affects the userspace but not expressed in new set type revision."? Are we breaking ABI here by adding a new field to print out the # of elements in a hash? Would it be better to default this to off and only print it if a new flag were presented? I guess I don't see why we should rev all of the hash types for this change when we're not really adding any functionality to them. We can rev the ver of the library to signify a change here. Would that help? If we do need to add a new revision I think we may want to step back and make sure there's no other information we want to expose first and if there is do it all in one shot to minimize the # of revisions. Thanks for your help. Josh [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4566 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Add element count to hash headers 2015-03-04 3:34 ` Josh Hunt @ 2015-03-04 14:39 ` Eric B Munson 2015-03-06 21:45 ` Jozsef Kadlecsik 1 sibling, 0 replies; 7+ messages in thread From: Eric B Munson @ 2015-03-04 14:39 UTC (permalink / raw) To: Josh Hunt, Jozsef Kadlecsik; +Cc: netfilter-devel, Pablo Neira Ayuso -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03/03/2015 10:34 PM, Josh Hunt wrote: > On 02/18/2015 01:53 PM, Eric B Munson wrote: >> snip >> > > Jozsef > > Can you clarify what you're looking for when you mentioned (in a > previous mail) "I don't like any change which affects the userspace > but not expressed in new set type revision."? Are we breaking ABI > here by adding a new field to print out the # of elements in a > hash? Would it be better to default this to off and only print it > if a new flag were presented? To be clear, we aren't adding a new field. This field exists already and is not in use for hash sets. > > I guess I don't see why we should rev all of the hash types for > this change when we're not really adding any functionality to them. > We can rev the ver of the library to signify a change here. Would > that help? > > If we do need to add a new revision I think we may want to step > back and make sure there's no other information we want to expose > first and if there is do it all in one shot to minimize the # of > revisions. > > Thanks for your help. Josh > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJU9xkuAAoJELbVsDOpoOa9eFgP/1FEhgQw/zrHxVhsPTi1uYRz h1ftmpOgkgqJWEBsGoydOr+hbYATIgoU6O829jH84OKFQdoPCl00uqO7XXLB601C ii1OPxb57iHL9/FnbTAHuSKqBhzQPwUSWuICISLulgAMD25an5eckbnRR/i8h0SW kEbDitCBFRqyKRJZXSSS36oQuryd9j/pH2UYxE5vtQ9oiTW+MXxK7HvzDE6RTssz rpVW3lbPXnLucMt4te1V4iX7t7r0f7UVcQuQGSOSEm1jB+94+AIowaoUzJDNkC6E JMMERSOS9RI7LerlG+5iy13nFQgOcTwxX/Xfhy5CNl7v7w6biVzzS18Kf2qwdFmt yOM/InBpwecE8VoTM2tlP6WLqpsNEE/aaiA7vLiqKU+U7xSYBN6o+8w0FdpKtasx +TNqr7wfKBx/oyvZQSb13YQ89ci/gDeVczO51peoNhQn9zy+XY41DSLrdCPfa9HZ x1lkiazFP/x07WpHPKs+g4AuMNkQ0vTkowVkH3orcBRnqUxinngAsnkefXC1TZ96 L38bJXLkOC//YfE+i/RtzzuX5KN6rbiw//h5nM3m0bg6Ri7royZuAtgt2ZoYDM0m SmxD1bX7lqi46G4FvPMDCzD/NlelMNJkNWAlvsWFx1K5TC4jXQhDMIjDv9Mpz2PT vI1LkbGQ2JYLUGBNmTzz =pny3 -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Add element count to hash headers 2015-03-04 3:34 ` Josh Hunt 2015-03-04 14:39 ` Eric B Munson @ 2015-03-06 21:45 ` Jozsef Kadlecsik 2015-03-09 19:05 ` Eric B Munson 1 sibling, 1 reply; 7+ messages in thread From: Jozsef Kadlecsik @ 2015-03-06 21:45 UTC (permalink / raw) To: Josh Hunt; +Cc: Eric B Munson, netfilter-devel, Pablo Neira Ayuso Hi Josh, On Tue, 3 Mar 2015, Josh Hunt wrote: > On 02/18/2015 01:53 PM, Eric B Munson wrote: > > It would be useful for userspace to query the size of an ipset hash, > > however, this data is not exposed to userspace outside of counting the > > number of member entries. This patch uses the attribute > > IPSET_ATTR_ELEMENTS to indicate the size in the the header that is > > exported to userspace. This field is then printed by the userspace > > tool for hashes. > > > > Because it is only meaningful for hashes to report their size, the > > output is conditional on the set type. To do this checking the > > MATCH_TYPENAME macro was moved to utils.h. > > > > Signed-off-by: Eric B Munson <emunson@akamai.com> > > Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> > > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > > Cc: Josh Hunt <johunt@akamai.com> > > --- > > include/libipset/utils.h | 3 +++ > > kernel/net/netfilter/ipset/ip_set_hash_gen.h | 3 ++- > > lib/errcode.c | 2 -- > > lib/session.c | 14 ++++++++++++-- > > 4 files changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/include/libipset/utils.h b/include/libipset/utils.h > > index 3cd29da..ceedd45 100644 > > --- a/include/libipset/utils.h > > +++ b/include/libipset/utils.h > > @@ -19,6 +19,9 @@ > > #define STRCASEQ(a, b) (strcasecmp(a, b) == 0) > > #define STRNCASEQ(a, b, n) (strncasecmp(a, b, n) == 0) > > > > +/* Match set type names */ > > +#define MATCH_TYPENAME(a, b) STRNEQ(a, b, strlen(b)) > > + > > /* Stringify tokens */ > > #define _STR(c) #c > > #define STR(c) _STR(c) > > diff --git a/kernel/net/netfilter/ipset/ip_set_hash_gen.h > > b/kernel/net/netfilter/ipset/ip_set_hash_gen.h > > index 885105b..2000a20 100644 > > --- a/kernel/net/netfilter/ipset/ip_set_hash_gen.h > > +++ b/kernel/net/netfilter/ipset/ip_set_hash_gen.h > > @@ -1040,7 +1040,8 @@ mtype_head(struct ip_set *set, struct sk_buff *skb) > > goto nla_put_failure; > > #endif > > if (nla_put_net32(skb, IPSET_ATTR_REFERENCES, htonl(set->ref - 1)) || > > - nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize))) > > + nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize)) || > > + nla_put_net32(skb, IPSET_ATTR_ELEMENTS, htonl(h->elements))) > > goto nla_put_failure; > > if (unlikely(ip_set_put_flags(skb, set))) > > goto nla_put_failure; > > diff --git a/lib/errcode.c b/lib/errcode.c > > index 8eb275b..3881121 100644 > > --- a/lib/errcode.c > > +++ b/lib/errcode.c > > @@ -148,8 +148,6 @@ static const struct ipset_errcode_table > > list_errcode_table[] = { > > { }, > > }; > > > > -#define MATCH_TYPENAME(a, b) STRNEQ(a, b, strlen(b)) > > - > > /** > > * ipset_errcode - interpret a kernel error code > > * @session: session structure > > diff --git a/lib/session.c b/lib/session.c > > index 013d9d8..07f3396 100644 > > --- a/lib/session.c > > +++ b/lib/session.c > > @@ -931,6 +931,10 @@ list_create(struct ipset_session *session, struct > > nlattr *nla[]) > > safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); > > safe_snprintf(session, "\nReferences: "); > > safe_dprintf(session, ipset_print_number, > > IPSET_OPT_REFERENCES); > > + if (MATCH_TYPENAME(type->name , "hash:")) { > > + safe_snprintf(session, "\nNum Entries: "); It's just a minor thing, but please do not abbreviate. > > + safe_dprintf(session, ipset_print_number, > > IPSET_OPT_ELEMENTS); > > + } > > safe_snprintf(session, > > session->envopts & IPSET_ENV_LIST_HEADER ? > > "\n" : "\nMembers:\n"); > > @@ -940,10 +944,16 @@ list_create(struct ipset_session *session, struct > > nlattr *nla[]) > > safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); > > safe_snprintf(session, "</memsize>\n<references>"); > > safe_dprintf(session, ipset_print_number, > > IPSET_OPT_REFERENCES); > > + safe_snprintf(session, "</references>\n"); > > + if (MATCH_TYPENAME(type->name , "hash:")) { > > + safe_snprintf(session, "<numentries>"); > > + safe_dprintf(session, ipset_print_number, > > IPSET_OPT_ELEMENTS); > > + safe_snprintf(session, "</numentries>\n"); > > + } > > safe_snprintf(session, > > session->envopts & IPSET_ENV_LIST_HEADER ? > > - "</references>\n</header>\n" : > > - "</references>\n</header>\n<members>\n"); > > + "</header>\n" : > > + "</header>\n<members>\n"); > > break; > > default: > > break; > > > > Can you clarify what you're looking for when you mentioned (in a > previous mail) "I don't like any change which affects the userspace but > not expressed in new set type revision."? Are we breaking ABI here by > adding a new field to print out the # of elements in a hash? Would it be > better to default this to off and only print it if a new flag were > presented? Technically speaking there's nothing wrong with the patch. But I have a couple of small issues, actually all non-critical, still... The new revision for the set types may be an exaggregation. My concern was to make easier to "solve" reports like "Number of elements not listed, why?". If the new feature is expressed in a revision number (both in the kernel module of the set types and the userspace parts), then it's simple to say "check and compare the output of 'modinfo modulename' and 'ipset help'". Otherwise it's hard to figure out which part is missing the new feature: kernel, userspace or both. But it may be too much fussing on my part :-) The new line in the output "breaks" any script which parses the listing and not prepared that such change may occur (actually, the testsuite itself must be fixed therefore too). At the same time I don't really like the idea of a new flag to turn on the printing of the info - just print it. The listing becomes non-uniform and type-dependent, because it's restricted to the hash types. But therefore should we add the listing of the number of elements to the bitmap and list types too? For the hash types, the data comes free - for the other types is must be calculated at every listing. Also, the number of the elements may easily be wrong for sets with timeout: the counter does not take into account the just timed out entries but the listing of the elements does. Even if we check the timed out entries for the counter, entries may time out by the time it's they turn to be listed. At least it should be documented in the manpage. > I guess I don't see why we should rev all of the hash types for this > change when we're not really adding any functionality to them. We can > rev the ver of the library to signify a change here. Would that help? The revision counter is the best we have to express new features. It's not elegant at all. > If we do need to add a new revision I think we may want to step back and > make sure there's no other information we want to expose first and if > there is do it all in one shot to minimize the # of revisions. What kind of additional information do you have in mind? Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Add element count to hash headers 2015-03-06 21:45 ` Jozsef Kadlecsik @ 2015-03-09 19:05 ` Eric B Munson 2015-03-11 19:20 ` Jozsef Kadlecsik 0 siblings, 1 reply; 7+ messages in thread From: Eric B Munson @ 2015-03-09 19:05 UTC (permalink / raw) To: Jozsef Kadlecsik, Josh Hunt; +Cc: netfilter-devel, Pablo Neira Ayuso -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03/06/2015 04:45 PM, Jozsef Kadlecsik wrote: > Hi Josh, > > On Tue, 3 Mar 2015, Josh Hunt wrote: > >> On 02/18/2015 01:53 PM, Eric B Munson wrote: >>> It would be useful for userspace to query the size of an ipset >>> hash, however, this data is not exposed to userspace outside of >>> counting the number of member entries. This patch uses the >>> attribute IPSET_ATTR_ELEMENTS to indicate the size in the the >>> header that is exported to userspace. This field is then >>> printed by the userspace tool for hashes. >>> >>> Because it is only meaningful for hashes to report their size, >>> the output is conditional on the set type. To do this checking >>> the MATCH_TYPENAME macro was moved to utils.h. >>> >>> Signed-off-by: Eric B Munson <emunson@akamai.com> Cc: Jozsef >>> Kadlecsik <kadlec@blackhole.kfki.hu> Cc: Pablo Neira Ayuso >>> <pablo@netfilter.org> Cc: Josh Hunt <johunt@akamai.com> --- >>> include/libipset/utils.h | 3 +++ >>> kernel/net/netfilter/ipset/ip_set_hash_gen.h | 3 ++- >>> lib/errcode.c | 2 -- >>> lib/session.c | 14 >>> ++++++++++++-- 4 files changed, 17 insertions(+), 5 >>> deletions(-) >>> >>> diff --git a/include/libipset/utils.h >>> b/include/libipset/utils.h index 3cd29da..ceedd45 100644 --- >>> a/include/libipset/utils.h +++ b/include/libipset/utils.h @@ >>> -19,6 +19,9 @@ #define STRCASEQ(a, b) (strcasecmp(a, b) == 0) >>> #define STRNCASEQ(a, b, n) (strncasecmp(a, b, n) == 0) >>> >>> +/* Match set type names */ +#define MATCH_TYPENAME(a, b) >>> STRNEQ(a, b, strlen(b)) + /* Stringify tokens */ #define >>> _STR(c) #c #define STR(c) _STR(c) diff --git >>> a/kernel/net/netfilter/ipset/ip_set_hash_gen.h >>> b/kernel/net/netfilter/ipset/ip_set_hash_gen.h index >>> 885105b..2000a20 100644 --- >>> a/kernel/net/netfilter/ipset/ip_set_hash_gen.h +++ >>> b/kernel/net/netfilter/ipset/ip_set_hash_gen.h @@ -1040,7 >>> +1040,8 @@ mtype_head(struct ip_set *set, struct sk_buff *skb) >>> goto nla_put_failure; #endif if (nla_put_net32(skb, >>> IPSET_ATTR_REFERENCES, htonl(set->ref - 1)) || - >>> nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize))) + >>> nla_put_net32(skb, IPSET_ATTR_MEMSIZE, htonl(memsize)) || + >>> nla_put_net32(skb, IPSET_ATTR_ELEMENTS, htonl(h->elements))) >>> goto nla_put_failure; if (unlikely(ip_set_put_flags(skb, >>> set))) goto nla_put_failure; diff --git a/lib/errcode.c >>> b/lib/errcode.c index 8eb275b..3881121 100644 --- >>> a/lib/errcode.c +++ b/lib/errcode.c @@ -148,8 +148,6 @@ static >>> const struct ipset_errcode_table list_errcode_table[] = { { }, >>> }; >>> >>> -#define MATCH_TYPENAME(a, b) STRNEQ(a, b, strlen(b)) - /** * >>> ipset_errcode - interpret a kernel error code * @session: >>> session structure diff --git a/lib/session.c b/lib/session.c >>> index 013d9d8..07f3396 100644 --- a/lib/session.c +++ >>> b/lib/session.c @@ -931,6 +931,10 @@ list_create(struct >>> ipset_session *session, struct nlattr *nla[]) >>> safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); >>> safe_snprintf(session, "\nReferences: "); safe_dprintf(session, >>> ipset_print_number, IPSET_OPT_REFERENCES); + if >>> (MATCH_TYPENAME(type->name , "hash:")) { + >>> safe_snprintf(session, "\nNum Entries: "); > > It's just a minor thing, but please do not abbreviate. Will fix for V2 > >>> + safe_dprintf(session, ipset_print_number, >>> IPSET_OPT_ELEMENTS); + } safe_snprintf(session, >>> session->envopts & IPSET_ENV_LIST_HEADER ? "\n" : >>> "\nMembers:\n"); @@ -940,10 +944,16 @@ list_create(struct >>> ipset_session *session, struct nlattr *nla[]) >>> safe_dprintf(session, ipset_print_number, IPSET_OPT_MEMSIZE); >>> safe_snprintf(session, "</memsize>\n<references>"); >>> safe_dprintf(session, ipset_print_number, >>> IPSET_OPT_REFERENCES); + safe_snprintf(session, >>> "</references>\n"); + if (MATCH_TYPENAME(type->name , >>> "hash:")) { + safe_snprintf(session, "<numentries>"); + >>> safe_dprintf(session, ipset_print_number, IPSET_OPT_ELEMENTS); >>> + safe_snprintf(session, "</numentries>\n"); + } >>> safe_snprintf(session, session->envopts & IPSET_ENV_LIST_HEADER >>> ? - "</references>\n</header>\n" : - >>> "</references>\n</header>\n<members>\n"); + "</header>\n" : + >>> "</header>\n<members>\n"); break; default: break; >>> >> >> Can you clarify what you're looking for when you mentioned (in a >> previous mail) "I don't like any change which affects the >> userspace but not expressed in new set type revision."? Are we >> breaking ABI here by adding a new field to print out the # of >> elements in a hash? Would it be better to default this to off and >> only print it if a new flag were presented? Thanks for taking a look at the patch. > > Technically speaking there's nothing wrong with the patch. But I > have a couple of small issues, actually all non-critical, still... > > The new revision for the set types may be an exaggregation. My > concern was to make easier to "solve" reports like "Number of > elements not listed, why?". If the new feature is expressed in a > revision number (both in the kernel module of the set types and the > userspace parts), then it's simple to say "check and compare the > output of 'modinfo modulename' and 'ipset help'". Otherwise it's > hard to figure out which part is missing the new feature: kernel, > userspace or both. But it may be too much fussing on my part :-) > > The new line in the output "breaks" any script which parses the > listing and not prepared that such change may occur (actually, the > testsuite itself must be fixed therefore too). At the same time I > don't really like the idea of a new flag to turn on the printing of > the info - just print it. I will make sure that V2 includes any necessary changes to the test suite. > > The listing becomes non-uniform and type-dependent, because it's > restricted to the hash types. But therefore should we add the > listing of the number of elements to the bitmap and list types too? > For the hash types, the data comes free - for the other types is > must be calculated at every listing. This is the reason I only added it for the hash type. I don't have any objection to adding it to the bitmap and list types as well but I didn't have a consumer for that information in mind to justify the extra calculations at run time. Plus, the libipset consumer could count entries in output just as well as the library itself. > > Also, the number of the elements may easily be wrong for sets with > timeout: the counter does not take into account the just timed out > entries but the listing of the elements does. Even if we check the > timed out entries for the counter, entries may time out by the time > it's they turn to be listed. At least it should be documented in > the manpage. I will add a note in the manpage about this possible disconnect between the reported number of entries and the actual number in the result. > >> I guess I don't see why we should rev all of the hash types for >> this change when we're not really adding any functionality to >> them. We can rev the ver of the library to signify a change here. >> Would that help? > > The revision counter is the best we have to express new features. > It's not elegant at all. > >> If we do need to add a new revision I think we may want to step >> back and make sure there's no other information we want to expose >> first and if there is do it all in one shot to minimize the # of >> revisions. > > What kind of additional information do you have in mind? > > Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, > kadlecsik.jozsef@wigner.mta.hu PGP key : > http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner > Research Centre for Physics, Hungarian Academy of Sciences H-1525 > Budapest 114, POB. 49, Hungary > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJU/e7jAAoJELbVsDOpoOa9IKIP/0+ULjC3VWtMJC75L5DODskZ MeOIusxIE64I3o9GhALOqJk86OWfAPOL5IaSnYk7EM/z+1IGQUsicWz6VYSICTg/ so4xnpPFlmQo8A+iSPjXqVBuElVqvbA3nz2QHqiqZDoW1rNgvyV6Xmp2elI/XUuv xvbUyAi21Ld0rhHvg9+APTiIDBPe0bfnH7GS9uoKPaoEPrmOysKLLX8bFvS0Xhr+ keN1VNbDsS8k57dD38BS1I5d4ZXGDPYTYLnctoUeDg2CfcEdHKYL0VtTzEgorbgv VQYLKre4T59yWA4aPoGvjWCg0K9J18VduZp5FDxIt5o2sdtmJ37NQ9hjEyt+xuAS 30SoGc295jLLJwc7yePtSxSFymZB0t+uTTp7n0dYfjteClQg8/FLtqRvCiz3ISDa dFYURfawg18UC+OuCmtdfbMu7RuebDF2R/w7xx40OBNNYntrdYz7bU+tjctDZLf/ J2+VdIuUsH4AsYZtLgYK3Bj1a6KeP4yEbzdieD8UuzJmosRdx0L8Lm8fpOmQG7Cs 2i/MHZQbjkVeHRL5oaBdhGyqhCBMHknrXttMKz7stHbV8uPbnsy/cVo0vT8EXS1a uxLmSil0r9wvMXAxOkh4lfsM68wnWevsuJHKm+8hgoB5ROHmRBoaxQca0sN8i8IW 3i7ieVyOAySL81PHB6V/ =kxFv -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Add element count to hash headers 2015-03-09 19:05 ` Eric B Munson @ 2015-03-11 19:20 ` Jozsef Kadlecsik 2015-03-19 17:38 ` Eric B Munson 0 siblings, 1 reply; 7+ messages in thread From: Jozsef Kadlecsik @ 2015-03-11 19:20 UTC (permalink / raw) To: Eric B Munson; +Cc: Josh Hunt, netfilter-devel, Pablo Neira Ayuso On Mon, 9 Mar 2015, Eric B Munson wrote: > > The new line in the output "breaks" any script which parses the > > listing and not prepared that such change may occur (actually, the > > testsuite itself must be fixed therefore too). At the same time I > > don't really like the idea of a new flag to turn on the printing of > > the info - just print it. > > I will make sure that V2 includes any necessary changes to the test suite. Thanks in advance. > > The listing becomes non-uniform and type-dependent, because it's > > restricted to the hash types. But therefore should we add the > > listing of the number of elements to the bitmap and list types too? > > For the hash types, the data comes free - for the other types is > > must be calculated at every listing. > > This is the reason I only added it for the hash type. I don't have > any objection to adding it to the bitmap and list types as well but I > didn't have a consumer for that information in mind to justify the > extra calculations at run time. Plus, the libipset consumer could > count entries in output just as well as the library itself. That would be a little complicated (I mean to count the entries from libipset): the library should cache all the entries to count them, finish printing the header by the element count and then print the elements. But "ipset" supports the terse list mode when just the header is listed, without the elements. So I believe it's just better to calculate the element count in the kernel header function for the bitmap and list types as well. It can be a lazy counting, without taking into account the possible timed out entries: that way the element counter is at least consistent. Let's skip bumping the revision numbers for the set types. Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Add element count to hash headers 2015-03-11 19:20 ` Jozsef Kadlecsik @ 2015-03-19 17:38 ` Eric B Munson 0 siblings, 0 replies; 7+ messages in thread From: Eric B Munson @ 2015-03-19 17:38 UTC (permalink / raw) To: Jozsef Kadlecsik; +Cc: Josh Hunt, netfilter-devel, Pablo Neira Ayuso -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03/11/2015 03:20 PM, Jozsef Kadlecsik wrote: > On Mon, 9 Mar 2015, Eric B Munson wrote: > >>> The new line in the output "breaks" any script which parses the >>> listing and not prepared that such change may occur (actually, >>> the testsuite itself must be fixed therefore too). At the same >>> time I don't really like the idea of a new flag to turn on the >>> printing of the info - just print it. >> >> I will make sure that V2 includes any necessary changes to the >> test suite. > > Thanks in advance. > >>> The listing becomes non-uniform and type-dependent, because >>> it's restricted to the hash types. But therefore should we add >>> the listing of the number of elements to the bitmap and list >>> types too? For the hash types, the data comes free - for the >>> other types is must be calculated at every listing. >> >> This is the reason I only added it for the hash type. I don't >> have any objection to adding it to the bitmap and list types as >> well but I didn't have a consumer for that information in mind >> to justify the extra calculations at run time. Plus, the >> libipset consumer could count entries in output just as well as >> the library itself. > > That would be a little complicated (I mean to count the entries > from libipset): the library should cache all the entries to count > them, finish printing the header by the element count and then > print the elements. > > But "ipset" supports the terse list mode when just the header is > listed, without the elements. So I believe it's just better to > calculate the element count in the kernel header function for the > bitmap and list types as well. It can be a lazy counting, without > taking into account the possible timed out entries: that way the > element counter is at least consistent. > > Let's skip bumping the revision numbers for the set types. > I finally have V2 for the hash type ready to go. I ran into a number of snags with the test suite, so I am not sure that the new patch will cover all of it. These problems exist when I test master as well so I am at least sure that I did not introduce them. I am going to look into the test suite failures and add the element count to the bitmaps in separate patches. Eric -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBAgAGBQJVCwmcAAoJELbVsDOpoOa97sUP/1blr7j5aqrqHRyTNe4DkwoG jucfToO8KmSRUGTYNMqcHh1L8eZa3xfHVIDU1CeTtHkOKGjuVjcdx6pby91gPQwr PcWH+CKAldXhtOywJZf3P6L/gKgkIk/ssBImyIdAD+2eLISdscVH8dhupcYAoORz h+ye00tZASXiAKiVg/96DIi7n33BjGV4WpHVoGCMKlldsSzEX+2rdosNa7CNyHH/ Hl56jQSTQEX+GC0ilCy++IVz3J/etY+JFf1oJe3jk4XY+znlYalAWREc4bhrV2t9 /OANr+cy7cuxkdFESppB7vWke3OObdVwi1xIo/eAYvTyfMSjGBZ/GEFuOpAVwY+2 4U0VWBtBY8j9UNQA+emYBICQQj0Oh0uYjPS+OJgAeb1pFC4IdGYYzmY3eZ/vmHpZ g65F0xkXwGsf0elZJoeYBV6GRZTDiQaJ2MBEVpBOEjDVpxvNgRT53xlZ/FHYBL9f KAGKa01VcgHnbAw8PCD9lietPnubC7fLtxKqdldnlJBX0yORzEwcyNLpR34yhZR0 1P+QV4geAQbvmovyRF9rPzYVhRzBC0VYdV5Y/6hoZURTKlh0klSmsLvbxnMrl5DQ H2G72shyhJWn7qIivRSnngjPTnFWsBfol0uVzkxI/6xhAx/29ffR124DbuYCSCHL xxGLHvQb8/EsXPo4C9d+ =af5L -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-19 17:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-18 19:53 [PATCH RESEND] Add element count to hash headers Eric B Munson 2015-03-04 3:34 ` Josh Hunt 2015-03-04 14:39 ` Eric B Munson 2015-03-06 21:45 ` Jozsef Kadlecsik 2015-03-09 19:05 ` Eric B Munson 2015-03-11 19:20 ` Jozsef Kadlecsik 2015-03-19 17:38 ` Eric B Munson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.