Today when finalizing a transaction the number of node quota is checked
to not being exceeded after the transaction. This check is always done,
even if the transaction is being performed by a privileged connection,
or if there were no nodes created in the transaction.

Correct that by checking quota only if:
- the transaction is being performed by an unprivileged guest, and
- at least one node was created in the transaction

Reported-by: Julien Grall <julien@xen.org>
Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c        |  3 +++
 tools/xenstore/xenstored_domain.c      |  4 ++--
 tools/xenstore/xenstored_domain.h      |  2 +-
 tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
 tools/xenstore/xenstored_transaction.h |  3 +++
 5 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index a61db2db2d..3ca68681e3 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1472,6 +1472,9 @@ static struct node *create_node(struct connection *conn, const void *ctx,
 	if (!node)
 		return NULL;
 
+	if (conn && conn->transaction)
+		ta_node_created(conn->transaction);
+
 	node->data = data;
 	node->datalen = datalen;
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index d7fc2fafc7..f62be2245c 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -544,7 +544,7 @@ static struct domain *find_domain_by_domid(unsigned int domid)
 	return (d && d->introduced) ? d : NULL;
 }
 
-int acc_fix_domains(struct list_head *head, bool update)
+int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
 {
 	struct changed_domain *cd;
 	int cnt;
@@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool update)
 	list_for_each_entry(cd, head, list) {
 		cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
 		if (!update) {
-			if (cnt >= quota_nb_entry_per_domain)
+			if (chk_quota && cnt >= quota_nb_entry_per_domain)
 				return ENOSPC;
 			if (cnt < 0)
 				return ENOMEM;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index dc4660861e..ec6aa00cc7 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -96,7 +96,7 @@ void domain_outstanding_dec(struct connection *conn);
 void domain_outstanding_domid_dec(unsigned int domid);
 int domain_get_quota(const void *ctx, struct connection *conn,
 		     unsigned int domid);
-int acc_fix_domains(struct list_head *head, bool update);
+int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
 
 /* Write rate limiting */
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 1aa9d3cb3d..2b15506953 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -160,12 +160,20 @@ struct transaction
 	/* List of changed domains - to record the changed domain entry number */
 	struct list_head changed_domains;
 
+	/* There was at least one node created in the transaction. */
+	bool node_created;
+
 	/* Flag for letting transaction fail. */
 	bool fail;
 };
 
 uint64_t generation;
 
+void ta_node_created(struct transaction *trans)
+{
+	trans->node_created = true;
+}
+
 static struct accessed_node *find_accessed_node(struct transaction *trans,
 						const char *name)
 {
@@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct connection *conn,
 	const char *arg = onearg(in);
 	struct transaction *trans;
 	bool is_corrupt = false;
+	bool chk_quota;
 	int ret;
 
 	if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
@@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct connection *conn,
 	if (!conn->transaction_started)
 		conn->ta_start_time = 0;
 
+	chk_quota = trans->node_created && domain_is_unprivileged(conn);
+
 	/* Attach transaction to ctx for auto-cleanup */
 	talloc_steal(ctx, trans);
 
 	if (streq(arg, "T")) {
 		if (trans->fail)
 			return ENOMEM;
-		ret = acc_fix_domains(&trans->changed_domains, false);
+		ret = acc_fix_domains(&trans->changed_domains, chk_quota,
+				      false);
 		if (ret)
 			return ret;
 		ret = finalize_transaction(conn, trans, &is_corrupt);
@@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct connection *conn,
 		wrl_apply_debit_trans_commit(conn);
 
 		/* fix domain entry for each changed domain */
-		acc_fix_domains(&trans->changed_domains, true);
+		acc_fix_domains(&trans->changed_domains, false, true);
 
 		if (is_corrupt)
 			corrupt(conn, "transaction inconsistency");
diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h
index b6f8cb7d0a..883145163f 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -36,6 +36,9 @@ int do_transaction_end(const void *ctx, struct connection *conn,
 
 struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
 
+/* Set flag for created node. */
+void ta_node_created(struct transaction *trans);
+
 /* This node was accessed. */
 int __must_check access_node(struct connection *conn, struct node *node,
                              enum node_access_type type, TDB_DATA *key);
-- 
2.35.3

Hi Juergen,

On 24/02/2023 15:58, Juergen Gross wrote:
> Today when finalizing a transaction the number of node quota is checked
> to not being exceeded after the transaction. This check is always done,
> even if the transaction is being performed by a privileged connection,
> or if there were no nodes created in the transaction.
> 
> Correct that by checking quota only if:
> - the transaction is being performed by an unprivileged guest, and
> - at least one node was created in the transaction
> 
> Reported-by: Julien Grall <julien@xen.org>
> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   tools/xenstore/xenstored_core.c        |  3 +++
>   tools/xenstore/xenstored_domain.c      |  4 ++--
>   tools/xenstore/xenstored_domain.h      |  2 +-
>   tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>   tools/xenstore/xenstored_transaction.h |  3 +++
>   5 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index a61db2db2d..3ca68681e3 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1472,6 +1472,9 @@ static struct node *create_node(struct connection *conn, const void *ctx,
>   	if (!node)
>   		return NULL;
>   
> +	if (conn && conn->transaction)
> +		ta_node_created(conn->transaction);
> +
>   	node->data = data;
>   	node->datalen = datalen;
>   
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index d7fc2fafc7..f62be2245c 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -544,7 +544,7 @@ static struct domain *find_domain_by_domid(unsigned int domid)
>   	return (d && d->introduced) ? d : NULL;
>   }
>   
> -int acc_fix_domains(struct list_head *head, bool update)
> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
>   {
>   	struct changed_domain *cd;
>   	int cnt;
> @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool update)
>   	list_for_each_entry(cd, head, list) {
>   		cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>   		if (!update) {
> -			if (cnt >= quota_nb_entry_per_domain)
> +			if (chk_quota && cnt >= quota_nb_entry_per_domain)
>   				return ENOSPC;
>   			if (cnt < 0)
>   				return ENOMEM;
> diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
> index dc4660861e..ec6aa00cc7 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -96,7 +96,7 @@ void domain_outstanding_dec(struct connection *conn);
>   void domain_outstanding_domid_dec(unsigned int domid);
>   int domain_get_quota(const void *ctx, struct connection *conn,
>   		     unsigned int domid);
> -int acc_fix_domains(struct list_head *head, bool update);
> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);

Depending on the answer below, I would suggest to write that 'chk_quota' 
is ignored when ``update`` is true.

>   
>   /* Write rate limiting */
>   
> diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
> index 1aa9d3cb3d..2b15506953 100644
> --- a/tools/xenstore/xenstored_transaction.c
> +++ b/tools/xenstore/xenstored_transaction.c
> @@ -160,12 +160,20 @@ struct transaction
>   	/* List of changed domains - to record the changed domain entry number */
>   	struct list_head changed_domains;
>   
> +	/* There was at least one node created in the transaction. */
> +	bool node_created;
> +
>   	/* Flag for letting transaction fail. */
>   	bool fail;
>   };
>   
>   uint64_t generation;
>   
> +void ta_node_created(struct transaction *trans)
> +{
> +	trans->node_created = true;
> +}
> +
>   static struct accessed_node *find_accessed_node(struct transaction *trans,
>   						const char *name)
>   {
> @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct connection *conn,
>   	const char *arg = onearg(in);
>   	struct transaction *trans;
>   	bool is_corrupt = false;
> +	bool chk_quota;
>   	int ret;
>   
>   	if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
> @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct connection *conn,
>   	if (!conn->transaction_started)
>   		conn->ta_start_time = 0;
>   
> +	chk_quota = trans->node_created && domain_is_unprivileged(conn);
> +
>   	/* Attach transaction to ctx for auto-cleanup */
>   	talloc_steal(ctx, trans);
>   
>   	if (streq(arg, "T")) {
>   		if (trans->fail)
>   			return ENOMEM;
> -		ret = acc_fix_domains(&trans->changed_domains, false);
> +		ret = acc_fix_domains(&trans->changed_domains, chk_quota,
> +				      false);
>   		if (ret)
>   			return ret;
>   		ret = finalize_transaction(conn, trans, &is_corrupt);
> @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct connection *conn,
>   		wrl_apply_debit_trans_commit(conn);
>   
>   		/* fix domain entry for each changed domain */
> -		acc_fix_domains(&trans->changed_domains, true);
> +		acc_fix_domains(&trans->changed_domains, false, true);

In theory, shouldn't we pass 'chk_quota' rather than false? In practice, 
I know it doesn't make any difference between this is an update.

>   
>   		if (is_corrupt)
>   			corrupt(conn, "transaction inconsistency");
> diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h
> index b6f8cb7d0a..883145163f 100644
> --- a/tools/xenstore/xenstored_transaction.h
> +++ b/tools/xenstore/xenstored_transaction.h
> @@ -36,6 +36,9 @@ int do_transaction_end(const void *ctx, struct connection *conn,
>   
>   struct transaction *transaction_lookup(struct connection *conn, uint32_t id);
>   
> +/* Set flag for created node. */
> +void ta_node_created(struct transaction *trans);
> +
>   /* This node was accessed. */
>   int __must_check access_node(struct connection *conn, struct node *node,
>                                enum node_access_type type, TDB_DATA *key);

Cheers,

-- 
Julien Grall

On 23.03.23 13:38, Julien Grall wrote:
> Hi Juergen,
> 
> On 24/02/2023 15:58, Juergen Gross wrote:
>> Today when finalizing a transaction the number of node quota is checked
>> to not being exceeded after the transaction. This check is always done,
>> even if the transaction is being performed by a privileged connection,
>> or if there were no nodes created in the transaction.
>>
>> Correct that by checking quota only if:
>> - the transaction is being performed by an unprivileged guest, and
>> - at least one node was created in the transaction
>>
>> Reported-by: Julien Grall <julien@xen.org>
>> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/xenstore/xenstored_core.c        |  3 +++
>>   tools/xenstore/xenstored_domain.c      |  4 ++--
>>   tools/xenstore/xenstored_domain.h      |  2 +-
>>   tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>   tools/xenstore/xenstored_transaction.h |  3 +++
>>   5 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index a61db2db2d..3ca68681e3 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1472,6 +1472,9 @@ static struct node *create_node(struct connection *conn, 
>> const void *ctx,
>>       if (!node)
>>           return NULL;
>> +    if (conn && conn->transaction)
>> +        ta_node_created(conn->transaction);
>> +
>>       node->data = data;
>>       node->datalen = datalen;
>> diff --git a/tools/xenstore/xenstored_domain.c 
>> b/tools/xenstore/xenstored_domain.c
>> index d7fc2fafc7..f62be2245c 100644
>> --- a/tools/xenstore/xenstored_domain.c
>> +++ b/tools/xenstore/xenstored_domain.c
>> @@ -544,7 +544,7 @@ static struct domain *find_domain_by_domid(unsigned int 
>> domid)
>>       return (d && d->introduced) ? d : NULL;
>>   }
>> -int acc_fix_domains(struct list_head *head, bool update)
>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
>>   {
>>       struct changed_domain *cd;
>>       int cnt;
>> @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool update)
>>       list_for_each_entry(cd, head, list) {
>>           cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>>           if (!update) {
>> -            if (cnt >= quota_nb_entry_per_domain)
>> +            if (chk_quota && cnt >= quota_nb_entry_per_domain)
>>                   return ENOSPC;
>>               if (cnt < 0)
>>                   return ENOMEM;
>> diff --git a/tools/xenstore/xenstored_domain.h 
>> b/tools/xenstore/xenstored_domain.h
>> index dc4660861e..ec6aa00cc7 100644
>> --- a/tools/xenstore/xenstored_domain.h
>> +++ b/tools/xenstore/xenstored_domain.h
>> @@ -96,7 +96,7 @@ void domain_outstanding_dec(struct connection *conn);
>>   void domain_outstanding_domid_dec(unsigned int domid);
>>   int domain_get_quota(const void *ctx, struct connection *conn,
>>                unsigned int domid);
>> -int acc_fix_domains(struct list_head *head, bool update);
>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
> 
> Depending on the answer below, I would suggest to write that 'chk_quota' is 
> ignored when ``update`` is true.

With the answer below, do you agree that no additional comment is needed?
I'm fine either way.

> 
>>   /* Write rate limiting */
>> diff --git a/tools/xenstore/xenstored_transaction.c 
>> b/tools/xenstore/xenstored_transaction.c
>> index 1aa9d3cb3d..2b15506953 100644
>> --- a/tools/xenstore/xenstored_transaction.c
>> +++ b/tools/xenstore/xenstored_transaction.c
>> @@ -160,12 +160,20 @@ struct transaction
>>       /* List of changed domains - to record the changed domain entry number */
>>       struct list_head changed_domains;
>> +    /* There was at least one node created in the transaction. */
>> +    bool node_created;
>> +
>>       /* Flag for letting transaction fail. */
>>       bool fail;
>>   };
>>   uint64_t generation;
>> +void ta_node_created(struct transaction *trans)
>> +{
>> +    trans->node_created = true;
>> +}
>> +
>>   static struct accessed_node *find_accessed_node(struct transaction *trans,
>>                           const char *name)
>>   {
>> @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct connection 
>> *conn,
>>       const char *arg = onearg(in);
>>       struct transaction *trans;
>>       bool is_corrupt = false;
>> +    bool chk_quota;
>>       int ret;
>>       if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
>> @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct 
>> connection *conn,
>>       if (!conn->transaction_started)
>>           conn->ta_start_time = 0;
>> +    chk_quota = trans->node_created && domain_is_unprivileged(conn);
>> +
>>       /* Attach transaction to ctx for auto-cleanup */
>>       talloc_steal(ctx, trans);
>>       if (streq(arg, "T")) {
>>           if (trans->fail)
>>               return ENOMEM;
>> -        ret = acc_fix_domains(&trans->changed_domains, false);
>> +        ret = acc_fix_domains(&trans->changed_domains, chk_quota,
>> +                      false);
>>           if (ret)
>>               return ret;
>>           ret = finalize_transaction(conn, trans, &is_corrupt);
>> @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct connection 
>> *conn,
>>           wrl_apply_debit_trans_commit(conn);
>>           /* fix domain entry for each changed domain */
>> -        acc_fix_domains(&trans->changed_domains, true);
>> +        acc_fix_domains(&trans->changed_domains, false, true);
> 
> In theory, shouldn't we pass 'chk_quota' rather than false? In practice, I know 
> it doesn't make any difference between this is an update.

We explicitly don't want to check quota in the "update" case. So specifying
"false" is the correct thing to do IMHO.


Juergen

Hi,

On 23/03/2023 12:53, Juergen Gross wrote:
> On 23.03.23 13:38, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 24/02/2023 15:58, Juergen Gross wrote:
>>> Today when finalizing a transaction the number of node quota is checked
>>> to not being exceeded after the transaction. This check is always done,
>>> even if the transaction is being performed by a privileged connection,
>>> or if there were no nodes created in the transaction.
>>>
>>> Correct that by checking quota only if:
>>> - the transaction is being performed by an unprivileged guest, and
>>> - at least one node was created in the transaction
>>>
>>> Reported-by: Julien Grall <julien@xen.org>
>>> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>   tools/xenstore/xenstored_core.c        |  3 +++
>>>   tools/xenstore/xenstored_domain.c      |  4 ++--
>>>   tools/xenstore/xenstored_domain.h      |  2 +-
>>>   tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>>   tools/xenstore/xenstored_transaction.h |  3 +++
>>>   5 files changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_core.c 
>>> b/tools/xenstore/xenstored_core.c
>>> index a61db2db2d..3ca68681e3 100644
>>> --- a/tools/xenstore/xenstored_core.c
>>> +++ b/tools/xenstore/xenstored_core.c
>>> @@ -1472,6 +1472,9 @@ static struct node *create_node(struct 
>>> connection *conn, const void *ctx,
>>>       if (!node)
>>>           return NULL;
>>> +    if (conn && conn->transaction)
>>> +        ta_node_created(conn->transaction);
>>> +
>>>       node->data = data;
>>>       node->datalen = datalen;
>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>> b/tools/xenstore/xenstored_domain.c
>>> index d7fc2fafc7..f62be2245c 100644
>>> --- a/tools/xenstore/xenstored_domain.c
>>> +++ b/tools/xenstore/xenstored_domain.c
>>> @@ -544,7 +544,7 @@ static struct domain 
>>> *find_domain_by_domid(unsigned int domid)
>>>       return (d && d->introduced) ? d : NULL;
>>>   }
>>> -int acc_fix_domains(struct list_head *head, bool update)
>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool 
>>> update)
>>>   {
>>>       struct changed_domain *cd;
>>>       int cnt;
>>> @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool 
>>> update)
>>>       list_for_each_entry(cd, head, list) {
>>>           cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>>>           if (!update) {
>>> -            if (cnt >= quota_nb_entry_per_domain)
>>> +            if (chk_quota && cnt >= quota_nb_entry_per_domain)
>>>                   return ENOSPC;
>>>               if (cnt < 0)
>>>                   return ENOMEM;
>>> diff --git a/tools/xenstore/xenstored_domain.h 
>>> b/tools/xenstore/xenstored_domain.h
>>> index dc4660861e..ec6aa00cc7 100644
>>> --- a/tools/xenstore/xenstored_domain.h
>>> +++ b/tools/xenstore/xenstored_domain.h
>>> @@ -96,7 +96,7 @@ void domain_outstanding_dec(struct connection *conn);
>>>   void domain_outstanding_domid_dec(unsigned int domid);
>>>   int domain_get_quota(const void *ctx, struct connection *conn,
>>>                unsigned int domid);
>>> -int acc_fix_domains(struct list_head *head, bool update);
>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool 
>>> update);
>>
>> Depending on the answer below, I would suggest to write that 
>> 'chk_quota' is ignored when ``update`` is true.
> 
> With the answer below, do you agree that no additional comment is needed?
> I'm fine either way.
> 
>>
>>>   /* Write rate limiting */
>>> diff --git a/tools/xenstore/xenstored_transaction.c 
>>> b/tools/xenstore/xenstored_transaction.c
>>> index 1aa9d3cb3d..2b15506953 100644
>>> --- a/tools/xenstore/xenstored_transaction.c
>>> +++ b/tools/xenstore/xenstored_transaction.c
>>> @@ -160,12 +160,20 @@ struct transaction
>>>       /* List of changed domains - to record the changed domain entry 
>>> number */
>>>       struct list_head changed_domains;
>>> +    /* There was at least one node created in the transaction. */
>>> +    bool node_created;
>>> +
>>>       /* Flag for letting transaction fail. */
>>>       bool fail;
>>>   };
>>>   uint64_t generation;
>>> +void ta_node_created(struct transaction *trans)
>>> +{
>>> +    trans->node_created = true;
>>> +}
>>> +
>>>   static struct accessed_node *find_accessed_node(struct transaction 
>>> *trans,
>>>                           const char *name)
>>>   {
>>> @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct 
>>> connection *conn,
>>>       const char *arg = onearg(in);
>>>       struct transaction *trans;
>>>       bool is_corrupt = false;
>>> +    bool chk_quota;
>>>       int ret;
>>>       if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
>>> @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct 
>>> connection *conn,
>>>       if (!conn->transaction_started)
>>>           conn->ta_start_time = 0;
>>> +    chk_quota = trans->node_created && domain_is_unprivileged(conn);
>>> +
>>>       /* Attach transaction to ctx for auto-cleanup */
>>>       talloc_steal(ctx, trans);
>>>       if (streq(arg, "T")) {
>>>           if (trans->fail)
>>>               return ENOMEM;
>>> -        ret = acc_fix_domains(&trans->changed_domains, false);
>>> +        ret = acc_fix_domains(&trans->changed_domains, chk_quota,
>>> +                      false);
>>>           if (ret)
>>>               return ret;
>>>           ret = finalize_transaction(conn, trans, &is_corrupt);
>>> @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct 
>>> connection *conn,
>>>           wrl_apply_debit_trans_commit(conn);
>>>           /* fix domain entry for each changed domain */
>>> -        acc_fix_domains(&trans->changed_domains, true);
>>> +        acc_fix_domains(&trans->changed_domains, false, true);
>>
>> In theory, shouldn't we pass 'chk_quota' rather than false? In 
>> practice, I know it doesn't make any difference between this is an 
>> update.
> 
> We explicitly don't want to check quota in the "update" case. So specifying
> "false" is the correct thing to do IMHO.

Let me rephrase my comment differently. What would happen if the user 
pass 'true'? Would we check the quota or not?

I suspect this is a no, hence why I was suggested the comment to say the 
field is ignored. Alternatively, we could add an assert that ensure that 
chk_quota is false when update is true.

Cheers,

-- 
Julien Grall

On 23.03.23 15:20, Julien Grall wrote:
> Hi,
> 
> On 23/03/2023 12:53, Juergen Gross wrote:
>> On 23.03.23 13:38, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 24/02/2023 15:58, Juergen Gross wrote:
>>>> Today when finalizing a transaction the number of node quota is checked
>>>> to not being exceeded after the transaction. This check is always done,
>>>> even if the transaction is being performed by a privileged connection,
>>>> or if there were no nodes created in the transaction.
>>>>
>>>> Correct that by checking quota only if:
>>>> - the transaction is being performed by an unprivileged guest, and
>>>> - at least one node was created in the transaction
>>>>
>>>> Reported-by: Julien Grall <julien@xen.org>
>>>> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>   tools/xenstore/xenstored_core.c        |  3 +++
>>>>   tools/xenstore/xenstored_domain.c      |  4 ++--
>>>>   tools/xenstore/xenstored_domain.h      |  2 +-
>>>>   tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>>>   tools/xenstore/xenstored_transaction.h |  3 +++
>>>>   5 files changed, 23 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>>>> index a61db2db2d..3ca68681e3 100644
>>>> --- a/tools/xenstore/xenstored_core.c
>>>> +++ b/tools/xenstore/xenstored_core.c
>>>> @@ -1472,6 +1472,9 @@ static struct node *create_node(struct connection 
>>>> *conn, const void *ctx,
>>>>       if (!node)
>>>>           return NULL;
>>>> +    if (conn && conn->transaction)
>>>> +        ta_node_created(conn->transaction);
>>>> +
>>>>       node->data = data;
>>>>       node->datalen = datalen;
>>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>>> b/tools/xenstore/xenstored_domain.c
>>>> index d7fc2fafc7..f62be2245c 100644
>>>> --- a/tools/xenstore/xenstored_domain.c
>>>> +++ b/tools/xenstore/xenstored_domain.c
>>>> @@ -544,7 +544,7 @@ static struct domain *find_domain_by_domid(unsigned int 
>>>> domid)
>>>>       return (d && d->introduced) ? d : NULL;
>>>>   }
>>>> -int acc_fix_domains(struct list_head *head, bool update)
>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
>>>>   {
>>>>       struct changed_domain *cd;
>>>>       int cnt;
>>>> @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool update)
>>>>       list_for_each_entry(cd, head, list) {
>>>>           cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>>>>           if (!update) {
>>>> -            if (cnt >= quota_nb_entry_per_domain)
>>>> +            if (chk_quota && cnt >= quota_nb_entry_per_domain)
>>>>                   return ENOSPC;
>>>>               if (cnt < 0)
>>>>                   return ENOMEM;
>>>> diff --git a/tools/xenstore/xenstored_domain.h 
>>>> b/tools/xenstore/xenstored_domain.h
>>>> index dc4660861e..ec6aa00cc7 100644
>>>> --- a/tools/xenstore/xenstored_domain.h
>>>> +++ b/tools/xenstore/xenstored_domain.h
>>>> @@ -96,7 +96,7 @@ void domain_outstanding_dec(struct connection *conn);
>>>>   void domain_outstanding_domid_dec(unsigned int domid);
>>>>   int domain_get_quota(const void *ctx, struct connection *conn,
>>>>                unsigned int domid);
>>>> -int acc_fix_domains(struct list_head *head, bool update);
>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
>>>
>>> Depending on the answer below, I would suggest to write that 'chk_quota' is 
>>> ignored when ``update`` is true.
>>
>> With the answer below, do you agree that no additional comment is needed?
>> I'm fine either way.
>>
>>>
>>>>   /* Write rate limiting */
>>>> diff --git a/tools/xenstore/xenstored_transaction.c 
>>>> b/tools/xenstore/xenstored_transaction.c
>>>> index 1aa9d3cb3d..2b15506953 100644
>>>> --- a/tools/xenstore/xenstored_transaction.c
>>>> +++ b/tools/xenstore/xenstored_transaction.c
>>>> @@ -160,12 +160,20 @@ struct transaction
>>>>       /* List of changed domains - to record the changed domain entry number */
>>>>       struct list_head changed_domains;
>>>> +    /* There was at least one node created in the transaction. */
>>>> +    bool node_created;
>>>> +
>>>>       /* Flag for letting transaction fail. */
>>>>       bool fail;
>>>>   };
>>>>   uint64_t generation;
>>>> +void ta_node_created(struct transaction *trans)
>>>> +{
>>>> +    trans->node_created = true;
>>>> +}
>>>> +
>>>>   static struct accessed_node *find_accessed_node(struct transaction *trans,
>>>>                           const char *name)
>>>>   {
>>>> @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct 
>>>> connection *conn,
>>>>       const char *arg = onearg(in);
>>>>       struct transaction *trans;
>>>>       bool is_corrupt = false;
>>>> +    bool chk_quota;
>>>>       int ret;
>>>>       if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
>>>> @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct 
>>>> connection *conn,
>>>>       if (!conn->transaction_started)
>>>>           conn->ta_start_time = 0;
>>>> +    chk_quota = trans->node_created && domain_is_unprivileged(conn);
>>>> +
>>>>       /* Attach transaction to ctx for auto-cleanup */
>>>>       talloc_steal(ctx, trans);
>>>>       if (streq(arg, "T")) {
>>>>           if (trans->fail)
>>>>               return ENOMEM;
>>>> -        ret = acc_fix_domains(&trans->changed_domains, false);
>>>> +        ret = acc_fix_domains(&trans->changed_domains, chk_quota,
>>>> +                      false);
>>>>           if (ret)
>>>>               return ret;
>>>>           ret = finalize_transaction(conn, trans, &is_corrupt);
>>>> @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct 
>>>> connection *conn,
>>>>           wrl_apply_debit_trans_commit(conn);
>>>>           /* fix domain entry for each changed domain */
>>>> -        acc_fix_domains(&trans->changed_domains, true);
>>>> +        acc_fix_domains(&trans->changed_domains, false, true);
>>>
>>> In theory, shouldn't we pass 'chk_quota' rather than false? In practice, I 
>>> know it doesn't make any difference between this is an update.
>>
>> We explicitly don't want to check quota in the "update" case. So specifying
>> "false" is the correct thing to do IMHO.
> 
> Let me rephrase my comment differently. What would happen if the user pass 
> 'true'? Would we check the quota or not?
> 
> I suspect this is a no, hence why I was suggested the comment to say the field 
> is ignored. Alternatively, we could add an assert that ensure that chk_quota is 
> false when update is true.

Okay, I'll add the comment.


Juergen

On 23/03/2023 14:21, Juergen Gross wrote:
> On 23.03.23 15:20, Julien Grall wrote:
>> Hi,
>>
>> On 23/03/2023 12:53, Juergen Gross wrote:
>>> On 23.03.23 13:38, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 24/02/2023 15:58, Juergen Gross wrote:
>>>>> Today when finalizing a transaction the number of node quota is 
>>>>> checked
>>>>> to not being exceeded after the transaction. This check is always 
>>>>> done,
>>>>> even if the transaction is being performed by a privileged connection,
>>>>> or if there were no nodes created in the transaction.
>>>>>
>>>>> Correct that by checking quota only if:
>>>>> - the transaction is being performed by an unprivileged guest, and
>>>>> - at least one node was created in the transaction
>>>>>
>>>>> Reported-by: Julien Grall <julien@xen.org>
>>>>> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>>   tools/xenstore/xenstored_core.c        |  3 +++
>>>>>   tools/xenstore/xenstored_domain.c      |  4 ++--
>>>>>   tools/xenstore/xenstored_domain.h      |  2 +-
>>>>>   tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>>>>   tools/xenstore/xenstored_transaction.h |  3 +++
>>>>>   5 files changed, 23 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/xenstore/xenstored_core.c 
>>>>> b/tools/xenstore/xenstored_core.c
>>>>> index a61db2db2d..3ca68681e3 100644
>>>>> --- a/tools/xenstore/xenstored_core.c
>>>>> +++ b/tools/xenstore/xenstored_core.c
>>>>> @@ -1472,6 +1472,9 @@ static struct node *create_node(struct 
>>>>> connection *conn, const void *ctx,
>>>>>       if (!node)
>>>>>           return NULL;
>>>>> +    if (conn && conn->transaction)
>>>>> +        ta_node_created(conn->transaction);
>>>>> +
>>>>>       node->data = data;
>>>>>       node->datalen = datalen;
>>>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>>>> b/tools/xenstore/xenstored_domain.c
>>>>> index d7fc2fafc7..f62be2245c 100644
>>>>> --- a/tools/xenstore/xenstored_domain.c
>>>>> +++ b/tools/xenstore/xenstored_domain.c
>>>>> @@ -544,7 +544,7 @@ static struct domain 
>>>>> *find_domain_by_domid(unsigned int domid)
>>>>>       return (d && d->introduced) ? d : NULL;
>>>>>   }
>>>>> -int acc_fix_domains(struct list_head *head, bool update)
>>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool 
>>>>> update)
>>>>>   {
>>>>>       struct changed_domain *cd;
>>>>>       int cnt;
>>>>> @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, 
>>>>> bool update)
>>>>>       list_for_each_entry(cd, head, list) {
>>>>>           cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>>>>>           if (!update) {
>>>>> -            if (cnt >= quota_nb_entry_per_domain)
>>>>> +            if (chk_quota && cnt >= quota_nb_entry_per_domain)
>>>>>                   return ENOSPC;
>>>>>               if (cnt < 0)
>>>>>                   return ENOMEM;
>>>>> diff --git a/tools/xenstore/xenstored_domain.h 
>>>>> b/tools/xenstore/xenstored_domain.h
>>>>> index dc4660861e..ec6aa00cc7 100644
>>>>> --- a/tools/xenstore/xenstored_domain.h
>>>>> +++ b/tools/xenstore/xenstored_domain.h
>>>>> @@ -96,7 +96,7 @@ void domain_outstanding_dec(struct connection 
>>>>> *conn);
>>>>>   void domain_outstanding_domid_dec(unsigned int domid);
>>>>>   int domain_get_quota(const void *ctx, struct connection *conn,
>>>>>                unsigned int domid);
>>>>> -int acc_fix_domains(struct list_head *head, bool update);
>>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool 
>>>>> update);
>>>>
>>>> Depending on the answer below, I would suggest to write that 
>>>> 'chk_quota' is ignored when ``update`` is true.
>>>
>>> With the answer below, do you agree that no additional comment is 
>>> needed?
>>> I'm fine either way.
>>>
>>>>
>>>>>   /* Write rate limiting */
>>>>> diff --git a/tools/xenstore/xenstored_transaction.c 
>>>>> b/tools/xenstore/xenstored_transaction.c
>>>>> index 1aa9d3cb3d..2b15506953 100644
>>>>> --- a/tools/xenstore/xenstored_transaction.c
>>>>> +++ b/tools/xenstore/xenstored_transaction.c
>>>>> @@ -160,12 +160,20 @@ struct transaction
>>>>>       /* List of changed domains - to record the changed domain 
>>>>> entry number */
>>>>>       struct list_head changed_domains;
>>>>> +    /* There was at least one node created in the transaction. */
>>>>> +    bool node_created;
>>>>> +
>>>>>       /* Flag for letting transaction fail. */
>>>>>       bool fail;
>>>>>   };
>>>>>   uint64_t generation;
>>>>> +void ta_node_created(struct transaction *trans)
>>>>> +{
>>>>> +    trans->node_created = true;
>>>>> +}
>>>>> +
>>>>>   static struct accessed_node *find_accessed_node(struct 
>>>>> transaction *trans,
>>>>>                           const char *name)
>>>>>   {
>>>>> @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct 
>>>>> connection *conn,
>>>>>       const char *arg = onearg(in);
>>>>>       struct transaction *trans;
>>>>>       bool is_corrupt = false;
>>>>> +    bool chk_quota;
>>>>>       int ret;
>>>>>       if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
>>>>> @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, 
>>>>> struct connection *conn,
>>>>>       if (!conn->transaction_started)
>>>>>           conn->ta_start_time = 0;
>>>>> +    chk_quota = trans->node_created && domain_is_unprivileged(conn);
>>>>> +
>>>>>       /* Attach transaction to ctx for auto-cleanup */
>>>>>       talloc_steal(ctx, trans);
>>>>>       if (streq(arg, "T")) {
>>>>>           if (trans->fail)
>>>>>               return ENOMEM;
>>>>> -        ret = acc_fix_domains(&trans->changed_domains, false);
>>>>> +        ret = acc_fix_domains(&trans->changed_domains, chk_quota,
>>>>> +                      false);
>>>>>           if (ret)
>>>>>               return ret;
>>>>>           ret = finalize_transaction(conn, trans, &is_corrupt);
>>>>> @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct 
>>>>> connection *conn,
>>>>>           wrl_apply_debit_trans_commit(conn);
>>>>>           /* fix domain entry for each changed domain */
>>>>> -        acc_fix_domains(&trans->changed_domains, true);
>>>>> +        acc_fix_domains(&trans->changed_domains, false, true);
>>>>
>>>> In theory, shouldn't we pass 'chk_quota' rather than false? In 
>>>> practice, I know it doesn't make any difference between this is an 
>>>> update.
>>>
>>> We explicitly don't want to check quota in the "update" case. So 
>>> specifying
>>> "false" is the correct thing to do IMHO.
>>
>> Let me rephrase my comment differently. What would happen if the user 
>> pass 'true'? Would we check the quota or not?
>>
>> I suspect this is a no, hence why I was suggested the comment to say 
>> the field is ignored. Alternatively, we could add an assert that 
>> ensure that chk_quota is false when update is true.
> 
> Okay, I'll add the comment.

Thanks! No need to send a new version. I tested the code and confirmed 
that it solved the problem I reported. I would be happy to add the 
comment on commit once we agree on it.

Cheers,

-- 
Julien Grall

On 23.03.23 15:29, Julien Grall wrote:
> 
> 
> On 23/03/2023 14:21, Juergen Gross wrote:
>> On 23.03.23 15:20, Julien Grall wrote:
>>> Hi,
>>>
>>> On 23/03/2023 12:53, Juergen Gross wrote:
>>>> On 23.03.23 13:38, Julien Grall wrote:
>>>>> Hi Juergen,
>>>>>
>>>>> On 24/02/2023 15:58, Juergen Gross wrote:
>>>>>> Today when finalizing a transaction the number of node quota is checked
>>>>>> to not being exceeded after the transaction. This check is always done,
>>>>>> even if the transaction is being performed by a privileged connection,
>>>>>> or if there were no nodes created in the transaction.
>>>>>>
>>>>>> Correct that by checking quota only if:
>>>>>> - the transaction is being performed by an unprivileged guest, and
>>>>>> - at least one node was created in the transaction
>>>>>>
>>>>>> Reported-by: Julien Grall <julien@xen.org>
>>>>>> Fixes: f2bebf72c4d5 ("xenstore: rework of transaction handling")
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>> ---
>>>>>>   tools/xenstore/xenstored_core.c        |  3 +++
>>>>>>   tools/xenstore/xenstored_domain.c      |  4 ++--
>>>>>>   tools/xenstore/xenstored_domain.h      |  2 +-
>>>>>>   tools/xenstore/xenstored_transaction.c | 16 ++++++++++++++--
>>>>>>   tools/xenstore/xenstored_transaction.h |  3 +++
>>>>>>   5 files changed, 23 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/xenstore/xenstored_core.c 
>>>>>> b/tools/xenstore/xenstored_core.c
>>>>>> index a61db2db2d..3ca68681e3 100644
>>>>>> --- a/tools/xenstore/xenstored_core.c
>>>>>> +++ b/tools/xenstore/xenstored_core.c
>>>>>> @@ -1472,6 +1472,9 @@ static struct node *create_node(struct connection 
>>>>>> *conn, const void *ctx,
>>>>>>       if (!node)
>>>>>>           return NULL;
>>>>>> +    if (conn && conn->transaction)
>>>>>> +        ta_node_created(conn->transaction);
>>>>>> +
>>>>>>       node->data = data;
>>>>>>       node->datalen = datalen;
>>>>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>>>>> b/tools/xenstore/xenstored_domain.c
>>>>>> index d7fc2fafc7..f62be2245c 100644
>>>>>> --- a/tools/xenstore/xenstored_domain.c
>>>>>> +++ b/tools/xenstore/xenstored_domain.c
>>>>>> @@ -544,7 +544,7 @@ static struct domain *find_domain_by_domid(unsigned 
>>>>>> int domid)
>>>>>>       return (d && d->introduced) ? d : NULL;
>>>>>>   }
>>>>>> -int acc_fix_domains(struct list_head *head, bool update)
>>>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
>>>>>>   {
>>>>>>       struct changed_domain *cd;
>>>>>>       int cnt;
>>>>>> @@ -552,7 +552,7 @@ int acc_fix_domains(struct list_head *head, bool update)
>>>>>>       list_for_each_entry(cd, head, list) {
>>>>>>           cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
>>>>>>           if (!update) {
>>>>>> -            if (cnt >= quota_nb_entry_per_domain)
>>>>>> +            if (chk_quota && cnt >= quota_nb_entry_per_domain)
>>>>>>                   return ENOSPC;
>>>>>>               if (cnt < 0)
>>>>>>                   return ENOMEM;
>>>>>> diff --git a/tools/xenstore/xenstored_domain.h 
>>>>>> b/tools/xenstore/xenstored_domain.h
>>>>>> index dc4660861e..ec6aa00cc7 100644
>>>>>> --- a/tools/xenstore/xenstored_domain.h
>>>>>> +++ b/tools/xenstore/xenstored_domain.h
>>>>>> @@ -96,7 +96,7 @@ void domain_outstanding_dec(struct connection *conn);
>>>>>>   void domain_outstanding_domid_dec(unsigned int domid);
>>>>>>   int domain_get_quota(const void *ctx, struct connection *conn,
>>>>>>                unsigned int domid);
>>>>>> -int acc_fix_domains(struct list_head *head, bool update);
>>>>>> +int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
>>>>>
>>>>> Depending on the answer below, I would suggest to write that 'chk_quota' is 
>>>>> ignored when ``update`` is true.
>>>>
>>>> With the answer below, do you agree that no additional comment is needed?
>>>> I'm fine either way.
>>>>
>>>>>
>>>>>>   /* Write rate limiting */
>>>>>> diff --git a/tools/xenstore/xenstored_transaction.c 
>>>>>> b/tools/xenstore/xenstored_transaction.c
>>>>>> index 1aa9d3cb3d..2b15506953 100644
>>>>>> --- a/tools/xenstore/xenstored_transaction.c
>>>>>> +++ b/tools/xenstore/xenstored_transaction.c
>>>>>> @@ -160,12 +160,20 @@ struct transaction
>>>>>>       /* List of changed domains - to record the changed domain entry 
>>>>>> number */
>>>>>>       struct list_head changed_domains;
>>>>>> +    /* There was at least one node created in the transaction. */
>>>>>> +    bool node_created;
>>>>>> +
>>>>>>       /* Flag for letting transaction fail. */
>>>>>>       bool fail;
>>>>>>   };
>>>>>>   uint64_t generation;
>>>>>> +void ta_node_created(struct transaction *trans)
>>>>>> +{
>>>>>> +    trans->node_created = true;
>>>>>> +}
>>>>>> +
>>>>>>   static struct accessed_node *find_accessed_node(struct transaction *trans,
>>>>>>                           const char *name)
>>>>>>   {
>>>>>> @@ -509,6 +517,7 @@ int do_transaction_end(const void *ctx, struct 
>>>>>> connection *conn,
>>>>>>       const char *arg = onearg(in);
>>>>>>       struct transaction *trans;
>>>>>>       bool is_corrupt = false;
>>>>>> +    bool chk_quota;
>>>>>>       int ret;
>>>>>>       if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
>>>>>> @@ -523,13 +532,16 @@ int do_transaction_end(const void *ctx, struct 
>>>>>> connection *conn,
>>>>>>       if (!conn->transaction_started)
>>>>>>           conn->ta_start_time = 0;
>>>>>> +    chk_quota = trans->node_created && domain_is_unprivileged(conn);
>>>>>> +
>>>>>>       /* Attach transaction to ctx for auto-cleanup */
>>>>>>       talloc_steal(ctx, trans);
>>>>>>       if (streq(arg, "T")) {
>>>>>>           if (trans->fail)
>>>>>>               return ENOMEM;
>>>>>> -        ret = acc_fix_domains(&trans->changed_domains, false);
>>>>>> +        ret = acc_fix_domains(&trans->changed_domains, chk_quota,
>>>>>> +                      false);
>>>>>>           if (ret)
>>>>>>               return ret;
>>>>>>           ret = finalize_transaction(conn, trans, &is_corrupt);
>>>>>> @@ -539,7 +551,7 @@ int do_transaction_end(const void *ctx, struct 
>>>>>> connection *conn,
>>>>>>           wrl_apply_debit_trans_commit(conn);
>>>>>>           /* fix domain entry for each changed domain */
>>>>>> -        acc_fix_domains(&trans->changed_domains, true);
>>>>>> +        acc_fix_domains(&trans->changed_domains, false, true);
>>>>>
>>>>> In theory, shouldn't we pass 'chk_quota' rather than false? In practice, I 
>>>>> know it doesn't make any difference between this is an update.
>>>>
>>>> We explicitly don't want to check quota in the "update" case. So specifying
>>>> "false" is the correct thing to do IMHO.
>>>
>>> Let me rephrase my comment differently. What would happen if the user pass 
>>> 'true'? Would we check the quota or not?
>>>
>>> I suspect this is a no, hence why I was suggested the comment to say the 
>>> field is ignored. Alternatively, we could add an assert that ensure that 
>>> chk_quota is false when update is true.
>>
>> Okay, I'll add the comment.
> 
> Thanks! No need to send a new version. I tested the code and confirmed that it 
> solved the problem I reported. I would be happy to add the comment on commit 
> once we agree on it.

Thanks,

Juergen