Instead of modifying accounting data and undo those modifications in
case of an error during further processing, add a framework for
collecting the needed changes and commit them only when the whole
operation has succeeded.

This scheme can reuse large parts of the per transaction accounting.
The changed_domain handling can be reused, but the array size of the
accounting data should be possible to be different for both use cases.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- call acc_commit() earlier (Julien Grall)
- add assert() to acc_commit()
- use fixed sized acc array in struct changed_domain (Julien Grall)
---
 tools/xenstore/xenstored_core.c   |  9 ++++--
 tools/xenstore/xenstored_core.h   |  3 ++
 tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++-
 tools/xenstore/xenstored_domain.h |  5 ++-
 4 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 3ca68681e3..84335f5f3d 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error)
 			break;
 		}
 	}
+
+	acc_drop(conn);
+
 	send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
 			  strlen(xsd_errors[i].errstring) + 1);
 }
@@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 
 	assert(type != XS_WATCH_EVENT);
 
+	conn->in = NULL;
+	acc_commit(conn);
+
 	if ( len > XENSTORE_PAYLOAD_MAX ) {
 		send_error(conn, E2BIG);
 		return;
@@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 		}
 	}
 
-	conn->in = NULL;
-
 	/* Update relevant header fields and fill in the message body. */
 	bdata->hdr.msg.type = type;
 	bdata->hdr.msg.len = len;
@@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct interface_funcs *funcs)
 	new->is_stalled = false;
 	new->transaction_started = 0;
 	INIT_LIST_HEAD(&new->out_list);
+	INIT_LIST_HEAD(&new->acc_list);
 	INIT_LIST_HEAD(&new->ref_list);
 	INIT_LIST_HEAD(&new->watches);
 	INIT_LIST_HEAD(&new->transaction_list);
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index c59b06551f..1f811f38cb 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -139,6 +139,9 @@ struct connection
 	struct list_head out_list;
 	uint64_t timeout_msec;
 
+	/* Not yet committed accounting data (valid if in != NULL). */
+	struct list_head acc_list;
+
 	/* Referenced requests no longer pending. */
 	struct list_head ref_list;
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 30fb9acec6..144cbafb73 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -91,6 +91,8 @@ struct domain
 	bool wrl_delay_logged;
 };
 
+#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N)
+
 struct changed_domain
 {
 	/* List of all changed domains. */
@@ -100,7 +102,7 @@ struct changed_domain
 	unsigned int domid;
 
 	/* Accounting data. */
-	int acc[ACC_TR_N];
+	int acc[ACC_CHD_N];
 };
 
 static struct hashtable *domhash;
@@ -1070,6 +1072,7 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 			  enum accitem what, int add, bool no_dom_alloc)
 {
 	struct domain *d;
+	struct changed_domain *cd;
 	struct list_head *head;
 	int ret;
 
@@ -1090,6 +1093,22 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 		}
 	}
 
+	/* Temporary accounting data until final commit? */
+	if (conn && conn->in && what < ACC_REQ_N) {
+		/* Consider transaction local data. */
+		ret = 0;
+		if (conn->transaction && what < ACC_TR_N) {
+			head = transaction_get_changed_domains(
+				conn->transaction);
+			cd = acc_find_changed_domain(head, domid);
+			if (cd)
+				ret = cd->acc[what];
+		}
+		ret += acc_add_changed_dom(conn->in, &conn->acc_list, what,
+					   add, domid);
+		return errno ? -1 : domain_acc_add_valid(d, what, ret);
+	}
+
 	if (conn && conn->transaction && what < ACC_TR_N) {
 		head = transaction_get_changed_domains(conn->transaction);
 		ret = acc_add_changed_dom(conn->transaction, head, what,
@@ -1106,6 +1125,38 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 	return d->acc[what];
 }
 
+void acc_drop(struct connection *conn)
+{
+	struct changed_domain *cd;
+
+	while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
+		list_del(&cd->list);
+		talloc_free(cd);
+	}
+}
+
+void acc_commit(struct connection *conn)
+{
+	struct changed_domain *cd;
+	enum accitem what;
+
+	/*
+	 * Make sure domain_acc_add() below can't add additional data to
+	 * to be committed accounting records.
+	 */
+	assert(!conn->in);
+
+	while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
+		list_del(&cd->list);
+		for (what = 0; what < ACC_REQ_N; what++)
+			if (cd->acc[what])
+				domain_acc_add(conn, cd->domid, what,
+					       cd->acc[what], true);
+
+		talloc_free(cd);
+	}
+}
+
 int domain_nbentry_inc(struct connection *conn, unsigned int domid)
 {
 	return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 9d05eb01da..6355ad4f37 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -25,7 +25,8 @@
  * a per transaction array.
  */
 enum accitem {
-	ACC_NODES,
+	ACC_REQ_N,		/* Number of elements per request. */
+	ACC_NODES = ACC_REQ_N,
 	ACC_TR_N,		/* Number of elements per transaction. */
 	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
 };
@@ -113,6 +114,8 @@ int domain_get_quota(const void *ctx, struct connection *conn,
  * If "update" is true, "chk_quota" is ignored.
  */
 int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
+void acc_drop(struct connection *conn);
+void acc_commit(struct connection *conn);
 
 /* Write rate limiting */
 
-- 
2.35.3

Hi Juergen,

On 05/04/2023 08:03, Juergen Gross wrote:
> Instead of modifying accounting data and undo those modifications in
> case of an error during further processing, add a framework for
> collecting the needed changes and commit them only when the whole
> operation has succeeded.
> 
> This scheme can reuse large parts of the per transaction accounting.
> The changed_domain handling can be reused, but the array size of the
> accounting data should be possible to be different for both use cases.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3:
> - call acc_commit() earlier (Julien Grall)
> - add assert() to acc_commit()
> - use fixed sized acc array in struct changed_domain (Julien Grall)
> ---
>   tools/xenstore/xenstored_core.c   |  9 ++++--
>   tools/xenstore/xenstored_core.h   |  3 ++
>   tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++-
>   tools/xenstore/xenstored_domain.h |  5 ++-
>   4 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 3ca68681e3..84335f5f3d 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error)
>   			break;
>   		}
>   	}
> +
> +	acc_drop(conn);
> +
>   	send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
>   			  strlen(xsd_errors[i].errstring) + 1);
>   }
> @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
>   
>   	assert(type != XS_WATCH_EVENT);
>   
> +	conn->in = NULL;

AFAIU, you are setting conn->in to NULL in order to please..

> +	acc_commit(conn);

... this call. However in case of an error like...

> +
>   	if ( len > XENSTORE_PAYLOAD_MAX ) { >   		send_error(conn, E2BIG);

... here, send_reply() will be called again. But the error will not be 
set because conn->in is NULL.

So I think you want to restore conn->in rewrite acc_commit(). The 
ordering would also deserve an explanation in a comment.

>   		return;
> @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
>   		}
>   	}
>   
> -	conn->in = NULL;
> -
>   	/* Update relevant header fields and fill in the message body. */
>   	bdata->hdr.msg.type = type;
>   	bdata->hdr.msg.len = len;
> @@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct interface_funcs *funcs)
>   	new->is_stalled = false;
>   	new->transaction_started = 0;
>   	INIT_LIST_HEAD(&new->out_list);
> +	INIT_LIST_HEAD(&new->acc_list);
>   	INIT_LIST_HEAD(&new->ref_list);
>   	INIT_LIST_HEAD(&new->watches);
>   	INIT_LIST_HEAD(&new->transaction_list);
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index c59b06551f..1f811f38cb 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -139,6 +139,9 @@ struct connection
>   	struct list_head out_list;
>   	uint64_t timeout_msec;
>   
> +	/* Not yet committed accounting data (valid if in != NULL). */
> +	struct list_head acc_list;
> +
>   	/* Referenced requests no longer pending. */
>   	struct list_head ref_list;
>   
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 30fb9acec6..144cbafb73 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -91,6 +91,8 @@ struct domain
>   	bool wrl_delay_logged;
>   };
>   
> +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N)

Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need this 
magic?

Related, wouldn't it be better to define it in the enum?

> +
>   struct changed_domain
>   {
>   	/* List of all changed domains. */
> @@ -100,7 +102,7 @@ struct changed_domain
>   	unsigned int domid;
>   
>   	/* Accounting data. */
> -	int acc[ACC_TR_N];
> +	int acc[ACC_CHD_N];
>   };
>   
>   static struct hashtable *domhash;
> @@ -1070,6 +1072,7 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
>   			  enum accitem what, int add, bool no_dom_alloc)
>   {
>   	struct domain *d;
> +	struct changed_domain *cd;
>   	struct list_head *head;
>   	int ret;
>   
> @@ -1090,6 +1093,22 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
>   		}
>   	}
>   
> +	/* Temporary accounting data until final commit? */
> +	if (conn && conn->in && what < ACC_REQ_N) {
> +		/* Consider transaction local data. */
> +		ret = 0;
> +		if (conn->transaction && what < ACC_TR_N) {
> +			head = transaction_get_changed_domains(
> +				conn->transaction);
> +			cd = acc_find_changed_domain(head, domid);
> +			if (cd)
> +				ret = cd->acc[what];
> +		}
> +		ret += acc_add_changed_dom(conn->in, &conn->acc_list, what,
> +					   add, domid);
> +		return errno ? -1 : domain_acc_add_valid(d, what, ret);
> +	}
> +
>   	if (conn && conn->transaction && what < ACC_TR_N) {
>   		head = transaction_get_changed_domains(conn->transaction);
>   		ret = acc_add_changed_dom(conn->transaction, head, what,
> @@ -1106,6 +1125,38 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
>   	return d->acc[what];
>   }
>   
> +void acc_drop(struct connection *conn)
> +{
> +	struct changed_domain *cd;
> +
> +	while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
> +		list_del(&cd->list);
> +		talloc_free(cd);
> +	}
> +}
> +
> +void acc_commit(struct connection *conn)
> +{
> +	struct changed_domain *cd;
> +	enum accitem what;
> +
> +	/*
> +	 * Make sure domain_acc_add() below can't add additional data to
> +	 * to be committed accounting records.
> +	 */
> +	assert(!conn->in);
> +
> +	while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
> +		list_del(&cd->list);
> +		for (what = 0; what < ACC_REQ_N; what++)
> +			if (cd->acc[what])
> +				domain_acc_add(conn, cd->domid, what,
> +					       cd->acc[what], true);
> +
> +		talloc_free(cd);
> +	}
> +}
> +
>   int domain_nbentry_inc(struct connection *conn, unsigned int domid)
>   {
>   	return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
> diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
> index 9d05eb01da..6355ad4f37 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -25,7 +25,8 @@
>    * a per transaction array.
>    */
>   enum accitem {
> -	ACC_NODES,
> +	ACC_REQ_N,		/* Number of elements per request. */
> +	ACC_NODES = ACC_REQ_N,
>   	ACC_TR_N,		/* Number of elements per transaction. */
>   	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
>   };
> @@ -113,6 +114,8 @@ int domain_get_quota(const void *ctx, struct connection *conn,
>    * If "update" is true, "chk_quota" is ignored.
>    */
>   int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
> +void acc_drop(struct connection *conn);
> +void acc_commit(struct connection *conn);
>   
>   /* Write rate limiting */
>   

Cheers,

-- 
Julien Grall

On 25.04.23 19:52, Julien Grall wrote:
> Hi Juergen,
> 
> On 05/04/2023 08:03, Juergen Gross wrote:
>> Instead of modifying accounting data and undo those modifications in
>> case of an error during further processing, add a framework for
>> collecting the needed changes and commit them only when the whole
>> operation has succeeded.
>>
>> This scheme can reuse large parts of the per transaction accounting.
>> The changed_domain handling can be reused, but the array size of the
>> accounting data should be possible to be different for both use cases.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V3:
>> - call acc_commit() earlier (Julien Grall)
>> - add assert() to acc_commit()
>> - use fixed sized acc array in struct changed_domain (Julien Grall)
>> ---
>>   tools/xenstore/xenstored_core.c   |  9 ++++--
>>   tools/xenstore/xenstored_core.h   |  3 ++
>>   tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++-
>>   tools/xenstore/xenstored_domain.h |  5 ++-
>>   4 files changed, 66 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index 3ca68681e3..84335f5f3d 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error)
>>               break;
>>           }
>>       }
>> +
>> +    acc_drop(conn);
>> +
>>       send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
>>                 strlen(xsd_errors[i].errstring) + 1);
>>   }
>> @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum 
>> xsd_sockmsg_type type,
>>       assert(type != XS_WATCH_EVENT);
>> +    conn->in = NULL;
> 
> AFAIU, you are setting conn->in to NULL in order to please..
> 
>> +    acc_commit(conn);
> 
> ... this call. However in case of an error like...
> 
>> +
>>       if ( len > XENSTORE_PAYLOAD_MAX ) { >           send_error(conn, E2BIG);
> 
> ... here, send_reply() will be called again. But the error will not be set 
> because conn->in is NULL.
> 
> So I think you want to restore conn->in rewrite acc_commit(). The ordering would 
> also deserve an explanation in a comment.

Just to make sure I understand you correctly (I have some difficulties
parsing "So I think you want to restore conn->in rewrite acc_commit()."
completely):

You are suggesting to move setting conn->in to NULL to acc_commit() and
to restore it before returning? I'm fine with that.

> 
>>           return;
>> @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum 
>> xsd_sockmsg_type type,
>>           }
>>       }
>> -    conn->in = NULL;
>> -
>>       /* Update relevant header fields and fill in the message body. */
>>       bdata->hdr.msg.type = type;
>>       bdata->hdr.msg.len = len;
>> @@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct 
>> interface_funcs *funcs)
>>       new->is_stalled = false;
>>       new->transaction_started = 0;
>>       INIT_LIST_HEAD(&new->out_list);
>> +    INIT_LIST_HEAD(&new->acc_list);
>>       INIT_LIST_HEAD(&new->ref_list);
>>       INIT_LIST_HEAD(&new->watches);
>>       INIT_LIST_HEAD(&new->transaction_list);
>> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
>> index c59b06551f..1f811f38cb 100644
>> --- a/tools/xenstore/xenstored_core.h
>> +++ b/tools/xenstore/xenstored_core.h
>> @@ -139,6 +139,9 @@ struct connection
>>       struct list_head out_list;
>>       uint64_t timeout_msec;
>> +    /* Not yet committed accounting data (valid if in != NULL). */
>> +    struct list_head acc_list;
>> +
>>       /* Referenced requests no longer pending. */
>>       struct list_head ref_list;
>> diff --git a/tools/xenstore/xenstored_domain.c 
>> b/tools/xenstore/xenstored_domain.c
>> index 30fb9acec6..144cbafb73 100644
>> --- a/tools/xenstore/xenstored_domain.c
>> +++ b/tools/xenstore/xenstored_domain.c
>> @@ -91,6 +91,8 @@ struct domain
>>       bool wrl_delay_logged;
>>   };
>> +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N)
> 
> Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need this magic?
> 
> Related, wouldn't it be better to define it in the enum?

I can do that, of course. I just didn't want to make the enum even more
complex. :-)

But with a comment this should be okay IMO.


Juergen

Hi,

On 26/04/2023 08:08, Juergen Gross wrote:
> On 25.04.23 19:52, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 05/04/2023 08:03, Juergen Gross wrote:
>>> Instead of modifying accounting data and undo those modifications in
>>> case of an error during further processing, add a framework for
>>> collecting the needed changes and commit them only when the whole
>>> operation has succeeded.
>>>
>>> This scheme can reuse large parts of the per transaction accounting.
>>> The changed_domain handling can be reused, but the array size of the
>>> accounting data should be possible to be different for both use cases.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V3:
>>> - call acc_commit() earlier (Julien Grall)
>>> - add assert() to acc_commit()
>>> - use fixed sized acc array in struct changed_domain (Julien Grall)
>>> ---
>>>   tools/xenstore/xenstored_core.c   |  9 ++++--
>>>   tools/xenstore/xenstored_core.h   |  3 ++
>>>   tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++-
>>>   tools/xenstore/xenstored_domain.h |  5 ++-
>>>   4 files changed, 66 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_core.c 
>>> b/tools/xenstore/xenstored_core.c
>>> index 3ca68681e3..84335f5f3d 100644
>>> --- a/tools/xenstore/xenstored_core.c
>>> +++ b/tools/xenstore/xenstored_core.c
>>> @@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, 
>>> int error)
>>>               break;
>>>           }
>>>       }
>>> +
>>> +    acc_drop(conn);
>>> +
>>>       send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
>>>                 strlen(xsd_errors[i].errstring) + 1);
>>>   }
>>> @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum 
>>> xsd_sockmsg_type type,
>>>       assert(type != XS_WATCH_EVENT);
>>> +    conn->in = NULL;
>>
>> AFAIU, you are setting conn->in to NULL in order to please..
>>
>>> +    acc_commit(conn);
>>
>> ... this call. However in case of an error like...
>>
>>> +
>>>       if ( len > XENSTORE_PAYLOAD_MAX ) { >           
>>> send_error(conn, E2BIG);
>>
>> ... here, send_reply() will be called again. But the error will not be 
>> set because conn->in is NULL.
>>
>> So I think you want to restore conn->in rewrite acc_commit(). The 
>> ordering would also deserve an explanation in a comment.
> 
> Just to make sure I understand you correctly (I have some difficulties
> parsing "So I think you want to restore conn->in rewrite acc_commit()."
> completely):

Hmmm... Not sure why I wrote "rewrite". I was meant to say that you want 
to restore conn->in after acc_commit() is called.

> 
> You are suggesting to move setting conn->in to NULL to acc_commit() and
> to restore it before returning? I'm fine with that.

Either that or what I wrote above. It depends on whether you expect 
other caller to be in the same situation.

> 
>>
>>>           return;
>>> @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum 
>>> xsd_sockmsg_type type,
>>>           }
>>>       }
>>> -    conn->in = NULL;
>>> -
>>>       /* Update relevant header fields and fill in the message body. */
>>>       bdata->hdr.msg.type = type;
>>>       bdata->hdr.msg.len = len;
>>> @@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct 
>>> interface_funcs *funcs)
>>>       new->is_stalled = false;
>>>       new->transaction_started = 0;
>>>       INIT_LIST_HEAD(&new->out_list);
>>> +    INIT_LIST_HEAD(&new->acc_list);
>>>       INIT_LIST_HEAD(&new->ref_list);
>>>       INIT_LIST_HEAD(&new->watches);
>>>       INIT_LIST_HEAD(&new->transaction_list);
>>> diff --git a/tools/xenstore/xenstored_core.h 
>>> b/tools/xenstore/xenstored_core.h
>>> index c59b06551f..1f811f38cb 100644
>>> --- a/tools/xenstore/xenstored_core.h
>>> +++ b/tools/xenstore/xenstored_core.h
>>> @@ -139,6 +139,9 @@ struct connection
>>>       struct list_head out_list;
>>>       uint64_t timeout_msec;
>>> +    /* Not yet committed accounting data (valid if in != NULL). */
>>> +    struct list_head acc_list;
>>> +
>>>       /* Referenced requests no longer pending. */
>>>       struct list_head ref_list;
>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>> b/tools/xenstore/xenstored_domain.c
>>> index 30fb9acec6..144cbafb73 100644
>>> --- a/tools/xenstore/xenstored_domain.c
>>> +++ b/tools/xenstore/xenstored_domain.c
>>> @@ -91,6 +91,8 @@ struct domain
>>>       bool wrl_delay_logged;
>>>   };
>>> +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N)
>>
>> Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need 
>> this magic?
>>
>> Related, wouldn't it be better to define it in the enum?
> 
> I can do that, of course. I just didn't want to make the enum even more
> complex. :-)

My concern is there is a disconnect between the enum and this macro. 
What would happen if we increase the enum past ACC_REQ_N/ACC_TR_N?
Would it be necessary to update ACC_CHD_N?

Cheers,

-- 
Julien Grall

On 27.04.23 18:32, Julien Grall wrote:
> Hi,
> 
> On 26/04/2023 08:08, Juergen Gross wrote:
>> On 25.04.23 19:52, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 05/04/2023 08:03, Juergen Gross wrote:
>>>> Instead of modifying accounting data and undo those modifications in
>>>> case of an error during further processing, add a framework for
>>>> collecting the needed changes and commit them only when the whole
>>>> operation has succeeded.
>>>>
>>>> This scheme can reuse large parts of the per transaction accounting.
>>>> The changed_domain handling can be reused, but the array size of the
>>>> accounting data should be possible to be different for both use cases.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V3:
>>>> - call acc_commit() earlier (Julien Grall)
>>>> - add assert() to acc_commit()
>>>> - use fixed sized acc array in struct changed_domain (Julien Grall)
>>>> ---
>>>>   tools/xenstore/xenstored_core.c   |  9 ++++--
>>>>   tools/xenstore/xenstored_core.h   |  3 ++
>>>>   tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++-
>>>>   tools/xenstore/xenstored_domain.h |  5 ++-
>>>>   4 files changed, 66 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>>>> index 3ca68681e3..84335f5f3d 100644
>>>> --- a/tools/xenstore/xenstored_core.c
>>>> +++ b/tools/xenstore/xenstored_core.c
>>>> @@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int 
>>>> error)
>>>>               break;
>>>>           }
>>>>       }
>>>> +
>>>> +    acc_drop(conn);
>>>> +
>>>>       send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
>>>>                 strlen(xsd_errors[i].errstring) + 1);
>>>>   }
>>>> @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum 
>>>> xsd_sockmsg_type type,
>>>>       assert(type != XS_WATCH_EVENT);
>>>> +    conn->in = NULL;
>>>
>>> AFAIU, you are setting conn->in to NULL in order to please..
>>>
>>>> +    acc_commit(conn);
>>>
>>> ... this call. However in case of an error like...
>>>
>>>> +
>>>>       if ( len > XENSTORE_PAYLOAD_MAX ) { > send_error(conn, E2BIG);
>>>
>>> ... here, send_reply() will be called again. But the error will not be set 
>>> because conn->in is NULL.
>>>
>>> So I think you want to restore conn->in rewrite acc_commit(). The ordering 
>>> would also deserve an explanation in a comment.
>>
>> Just to make sure I understand you correctly (I have some difficulties
>> parsing "So I think you want to restore conn->in rewrite acc_commit()."
>> completely):
> 
> Hmmm... Not sure why I wrote "rewrite". I was meant to say that you want to 
> restore conn->in after acc_commit() is called.
> 
>>
>> You are suggesting to move setting conn->in to NULL to acc_commit() and
>> to restore it before returning? I'm fine with that.
> 
> Either that or what I wrote above. It depends on whether you expect other caller 
> to be in the same situation.

I think it should be local to acc_commit(), as conn->in being NULL is a
requirement of the acc_commit() handling.

> 
>>
>>>
>>>>           return;
>>>> @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum 
>>>> xsd_sockmsg_type type,
>>>>           }
>>>>       }
>>>> -    conn->in = NULL;
>>>> -
>>>>       /* Update relevant header fields and fill in the message body. */
>>>>       bdata->hdr.msg.type = type;
>>>>       bdata->hdr.msg.len = len;
>>>> @@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct 
>>>> interface_funcs *funcs)
>>>>       new->is_stalled = false;
>>>>       new->transaction_started = 0;
>>>>       INIT_LIST_HEAD(&new->out_list);
>>>> +    INIT_LIST_HEAD(&new->acc_list);
>>>>       INIT_LIST_HEAD(&new->ref_list);
>>>>       INIT_LIST_HEAD(&new->watches);
>>>>       INIT_LIST_HEAD(&new->transaction_list);
>>>> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
>>>> index c59b06551f..1f811f38cb 100644
>>>> --- a/tools/xenstore/xenstored_core.h
>>>> +++ b/tools/xenstore/xenstored_core.h
>>>> @@ -139,6 +139,9 @@ struct connection
>>>>       struct list_head out_list;
>>>>       uint64_t timeout_msec;
>>>> +    /* Not yet committed accounting data (valid if in != NULL). */
>>>> +    struct list_head acc_list;
>>>> +
>>>>       /* Referenced requests no longer pending. */
>>>>       struct list_head ref_list;
>>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>>> b/tools/xenstore/xenstored_domain.c
>>>> index 30fb9acec6..144cbafb73 100644
>>>> --- a/tools/xenstore/xenstored_domain.c
>>>> +++ b/tools/xenstore/xenstored_domain.c
>>>> @@ -91,6 +91,8 @@ struct domain
>>>>       bool wrl_delay_logged;
>>>>   };
>>>> +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N)
>>>
>>> Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need this magic?
>>>
>>> Related, wouldn't it be better to define it in the enum?
>>
>> I can do that, of course. I just didn't want to make the enum even more
>> complex. :-)
> 
> My concern is there is a disconnect between the enum and this macro. What would 
> happen if we increase the enum past ACC_REQ_N/ACC_TR_N?

This expansion is happening in later patches.

> Would it be necessary to update ACC_CHD_N?

With the #define above: no.

With including it in the enum: depends on the values of ACC_REQ_N and ACC_TR_N.
ACC_CHD_N must always be equal to the larger one of both.

In my current V6 variant I've added it to the enum with a comment:

+       ACC_CHD_N = ACC_TR_N,   /* max(ACC_REQ_N, ACC_TR_N), for changed dom. */


Juergen