Hi Florian,
On Mon, Nov 12, 2012 at 01:30:53PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > int nfct_label_set(struct nf_conntrack *ct, const char *label);
> > >
> > > sets the label 'label' on the ct object.
> > >
> > > void nfct_label_unset(struct nf_conntrack *ct, const char *label);
> > >
> > > opposite, label is cleared if it was set.
> >
> > Can you use the existing nfct_attr_set/unset API for this?
>
> Don't see how, nfct_attr_unset clears the entire attribute.
> I admit that it would be nice if one could re-use existing api...
>
> Right now nfct_attr_set_l(.. ATTR_CONNLABELS..) can be used to assign
> the bit-vector directly (and _get can be used to retrieve the
> label bit-vector).
You could use a nfct_bitmask object and methods to modify it:
struct nfct_bitmask *nfct_bitmask_alloc(void)
nfct_bitmask_set_bit(struct nfct_bitmask *, uint16_t bit)
nfct_bitmask_test_bit(struct nfct_bitmask *, uint16_t bit)
nfct_bitmask_unset_bit(struct nfct_bitmask *, uint16_t bit)
Then you can attach the bitmask to the object:
nfct_set_attr(ct, ATTR_CONNLABELS, bitmask);
The setter would do the special handling internally by attaching the
bitmask label to the ct object.
> > > open question is how the library should do the mapping, i.e.
> > > should it hard-code a path to the mapping file (currently
> > > its /etc/xtables/connlabel.conf in my iptables-patch).
> >
> > Add a new parameter to nfct_label_open to indicate where the file that
> > contains the mapping is.
>
> I think that we should avoid it; else this might become a
> portability nightmare where applications end up
> trying half a zillion names (/etc/xtables, /usr/etc, /usr/local...)
We can provide some constant to recommend a path. My only concern is
that the hardcoded path may not exist in some filesystem, thus,
forcing to recompile the library.
Anyway, I think the main user for this will be the conntrack utility
and any netfilter utilities.
One more question below:
> > > void *nfct_label_open(void);
> > > void nfct_label_close(void *);
> > > int nfct_label_iterate(void *fp, char *buf, size_t buflen);
> > >
> > > so you could do
> > > void *h = nfct_label_open();
> > > int bit;
> > > while ((bit = nfct_label_iterate(h, bufm sizeof(buf))) > 0)
> > > printf("bit %d: %s\n", bit, buf);
> > >
> > > ?
> >
> > That seems fine to me.
>
> Thanks. I now have the following public API:
>
> /**
> * nfct_label_set_bit - set a bit in the label bitvector
> * \param ct pointer to a valid conntrack object
> * \param bit bit to set
> * returns 0 if the bit is now set
> */
> int nfct_label_set_bit(struct nf_conntrack *ct, uint16_t bit)
> {
> return __label_set_bit(ct, bit);
> }
>
> /**
> * nfct_label_unset_bit - clear label bit on a conntrack object
> * \param ct pointer to a valid conntrack object
> * \param bit bit to clear
> */
> void nfct_label_unset(struct nf_conntrack *ct, uint16_t bit)
> {
> __label_unset_bit(ct, bit);
> }
>
> /**
> * nfct_label_test_bit - determine wheter bit is set
> * \param ct pointer to a valid conntrack object
> * \param bit bit to query
> *
> * returns 0 on sucess and -1 if bit is not set.
> */
> int nfct_label_test_bit(struct nf_conntrack *ct, uint16_t bit)
> {
> return __label_test_bit(ct, bit);
> }
>
> /**
> * nfct_label_get_max - get highest bit set in label vector
> * \param ct pointer to a valid conntrack object
> * returns highest bit set, or -1 if no bits are set.
> */
> int nfct_label_get_max(struct nf_conntrack *ct)
> {
> return __label_get_max(ct);
> }
>
> /**
> * nfct_label_open
> * \param open the default bit-name mapping file for reading.
> *
> * returns a FILE handle to be passed to nfct_label_iterate.
> */
> void *nfct_label_open(void);
>
> /**
> * nfct_label_iterate
> * \param handle handle obtained from nfct_label_open
> * \param buf buffer where name will be stored
> * \param len size of the buffer
> *
> * returns the bit the name is mapped to, or -1 when no more
> * labels are defined. The handle should be closed via
> * fclose() once it is of no more interest.
> */
> int nfct_label_iterate(void *handle, char *buf, size_t len);
One question, what is void *handle? Is it hidding a FILE * or a cache
containing the bitmask?
You can define some:
struct nfct_label_handle;
in libnetfilter_conntrack.h, and keep the layout private by defining
it in some internal header.
I'd go for the cache-based approach, ie. store a lookup array in
memory extracted from the file.
> /**
> * @}
> */
>
> Since nfct_label_open() now returns FILE* userspace can provide other
> mapping files as well [ nfct_label_open just looks at the default location ].
>
> I've killed the functions that took a "const char *labelname" as
> argument; it's error prone to fiddle with hidden file-descriptor
> in the library (not thread-safe...).
>
> We could later add another library that provides more sophisticated
> stuff, such as a label<->bit cache.
>
> The patch that i currently have (untested, so I won't send it yet...)
> adds about 300 LOC, i would like to avoid bloating libnetfilter_conntrack
> more than that.
Examples files usually help to see how the API looks in action, if you
can attach one in the next round, in would be great.
Regards.
--
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
