-----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@xxxxxxxxxx> Cc: Jozsef
>>> Kadlecsik <kadlec@xxxxxxxxxxxxxxxxx> Cc: Pablo Neira Ayuso
>>> <pablo@xxxxxxxxxxxxx> Cc: Josh Hunt <johunt@xxxxxxxxxx> ---
>>> 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@xxxxxxxxxxxxxxxxx,
> kadlecsik.jozsef@xxxxxxxxxxxxx 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-----
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
