----------------------------------------
> Date: Tue, 29 Jul 2014 18:32:09 +0200
> From: pablo@xxxxxxxxxxxxx
> To: a.perevalov@xxxxxxxxxxx
> CC: kyungmin.park@xxxxxxxxxxx; hs81.go@xxxxxxxxxxx; netfilter-devel@xxxxxxxxxxxxxxx; alexey.perevalov@xxxxxxxxxxx; mathieu.poirier@xxxxxxxxxx
> Subject: Re: [PATCH] netfilter: nfnetlink_acct: use flag to reset counters
>
> On Tue, Jul 29, 2014 at 03:46:00PM +0400, Alexey Perevalov wrote:
> [...]
>>>>@@ -143,7 +157,9 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>>> if (nla_put_string(skb, NFACCT_NAME, acct->name))
>>>> goto nla_put_failure;
>>>>- if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
>>>>+ if (type == NFNL_MSG_ACCT_GET_CTRZERO &&
>>>>+ (!nfacct_flags || is_counters_reset(nfacct_flags, acct->flags) ||
>>>>+ is_quotas_reset(nfacct_flags, acct->flags))) {
>>>Replacing this:
>>>
>>> acct->flags & nfacct_flags == nfacct_flags
>>>
>>>I think it should be enough.
>>>
>> Yes, agree, but how to be with just counters without a quota,
>> at first look it should be
>> #define NFACCT_F_COUNTER ~(NFACCT_F_QUOTA_PKTS |
>> NFACCT_F_QUOTA_BYTES | NFACCT_F_OVERQUOTA)
>
> Right. I get the problem, what I proposed is not enough.
>
> We can extend this in a more flexible way to allow better filtering
> from userspace, eg.
>
> NFACCT_FLAGS_FILTER (nest)
> NFACCT_VALUE (u32)
> NFACCT_MASK (u32)
>
> The use the value and the mask to perform:
>
> value & filter->mask == filter->data
>
> Where filter comes from netlink cb->data.
>
> Please, see this thread:
>
> http://www.spinics.net/lists/netfilter-devel/msg32173.html
>
> The idea is very similar. We should only have one single key after
> your patch to filter by flags.
>
> Is this OK for your needs?
I think yes, due filter mechanism should be extendible. As an example,
also I don't want to receive not yet touched counters (which has 0 bytes).
>
>> Also I decided to put such condition not into nfnl_acct_fill_info,
>> but directly into nfnl_acct_dump, it will make this feature more
>> general,
>> client will get ability to filter it not only for reset command, but
>> also for list command, also nfnl_acct_fill_info is using in many
>> places, e.g.
>> just get command or nfnl_overquota_report.
>
> Right, the filtering should be generic for both list and reset
> commands.
>
>> And finally, I found strange approach for working with
>> NFACCT_F_OVERQUOTA (value 1 << 2, 4), in nfnl_acct_overquota the
>> test_and_set_bit function is used, which wants
>> Nth bit, and that Nth bit is 4, but not 2. Clearing is fine
>> clear_bit accept Nth bit as well,
>> but nfnl_acct_new gets from netlink attribute NFACCT_F_OVERQUOTA not
>> as offset value.
>
> That's a bug. Would you also send a separated patch for that, please?
Ok, but I need to know you opinion. My opinion is following:
if you have 1st, 2nd, 3rd bit for flags, you should use 1st, 2nd, 3rd, and using power of 2
not robust, I think bug was hidden due it was the latest bit. But on other hand, usage of set_bit and clear_bit looks
really perfect. So right now user/kernel space serialization is using NFACCT_F_QUOTA_PKTS|BYTES their values are 1 and 2, I think it's not too late to change the value of enum nfnl_acct_flags to sequential number, but not power of 2 and then use bit helper functions.
But you could stay on old used approach, power of 2 in enum and no bit helpers.
>
>> I took a look at it because of:
>> 1. It's not convenient to keep combined bit set for
>> NFACCT_F_COUNTER, because NFACCT_F_OVERQUOTA not what we have in
>> acct->flags. Of course if you not against such bit set.
>> 2. nlnf_overquata_report sends to user space _forth_ activated bit,
>> but not _second_ for NFACCT_F_OVERQUOTA, but
>> NFACCT_F_QUOTA_[BYTES|PKTS] was being sent as is. Output of the
>> nfacct, after overquota event occurred, not so clear, in case of
>> bytes quota it's just changing to package. It's not a big deal to
>> fix it to print "overquoted", but I feel it's better to use some
>> unified method to set bits.
>
> That's inconsistent and this of course needs a fix. Patch? Thanks
No problem.
--
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
