Hi Pablo,
On 8/24/19 6:29 PM, Pablo Neira Ayuso wrote:
> On Fri, Aug 23, 2019 at 08:28:46PM +0200, Fernando Fernandez Mancera wrote:
>> On 8/23/19 8:05 PM, Fernando Fernandez Mancera wrote:
>>>
>>>
>>> On 8/23/19 2:42 PM, Pablo Neira Ayuso wrote:
>>>> On Fri, Aug 23, 2019 at 02:41:42PM +0200, Pablo Neira Ayuso wrote:
>>>>> On Thu, Aug 22, 2019 at 06:48:26PM +0200, Fernando Fernandez Mancera
>>>>> wrote:
>>>>>> @@ -1405,10 +1409,16 @@ struct nft_trans_elem {
>>>>>>
>>>>>> struct nft_trans_obj {
>>>>>> struct nft_object *obj;
>>>>>> + struct nlattr **tb;
>>>>>
>>>>> Instead of annotatint tb[] on the object, you can probably add here:
>>>>>
>>>>> union {
>>>>> struct quota {
>>>>> uint64_t consumed;
>>>>> uint64_t quota;
>>>>> } quota;
>>>>> };
>>>>>
>>>>> So the initial update annotates the values in the transaction.
>>>>>
>>
>> If we follow that pattern then the indirection would need the
>> nft_trans_phase enum, the quota struct and also the tb[] as parameters
>> because in the preparation phase we always need the tb[] array.
>
> Right, so this is my next idea :-)
>
> For the update case, I'd suggest you use the existing 'obj' field in
> the transaction object. The idea would be to allocate a new object via
> nft_obj_init() from the update path. Hence, you can use the existing
> expr->ops->init() interface to parse the attributes - I find the
> existing parsing for ->update() a bit redundant.
>
> Then, from the commit path, you use the new ->update() interface to
> update the object accordingly taking this new object as input. I think
> you cannot update u64 quota like you do in this patch. On 32-bit
> arches, an assignment of u64 won't be atomic. So you have to use
> atomic64_set() and atomic64_read() to make sure that packet path does
> not observes an inconsistent state. BTW, once you have updated the
> existing object, you can just release the object in the transaction
> coming in this update. I think you will need a 'bool update' field on
> the transaction object, so the commit path knows how to handle the
> update.
>
I would need to add a second 'obj' field in the transaction object in
order to have the existing object pointer and the new one.
Also, right now we are not using atomic64_set() when initializing u64
quota is that a bug then? If it is, I could include a patch fixing it.
Thanks!
