tools/xenstored: potentially split trace_io() out message

[PATCH] tools/xenstored: potentially split trace_io() out message

Posted by Juergen Gross 5 months, 1 week ago

Today write_messages() will call trace_io() after having written the
complete message to the ring buffer or socket.

In case the message can't be written in one go, split it by writing
one trace entry when starting the write and one when finishing it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
This patch is meant to go on top of the patch "tools/xenstored: remove
\"-V\" command line option" in order to not lose any possible debug
information.
---
 tools/xenstored/core.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 9046b200bc..a14b240ed9 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -121,7 +121,7 @@ void trace(const char *fmt, ...)
 
 static void trace_io(const struct connection *conn,
 		     const struct buffered_data *data,
-		     int out)
+		     const char *type)
 {
 	unsigned int i;
 	time_t now;
@@ -134,8 +134,7 @@ static void trace_io(const struct connection *conn,
 	tm = localtime(&now);
 
 	trace("io: %s %p (d%u) %04d%02d%02d %02d:%02d:%02d %s (",
-	      out ? "OUT" : "IN", conn, conn->id,
-	      tm->tm_year + 1900, tm->tm_mon + 1,
+	      type, conn, conn->id, tm->tm_year + 1900, tm->tm_mon + 1,
 	      tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec,
 	      sockmsg_string(data->hdr.msg.type));
 	
@@ -321,43 +320,54 @@ static bool write_messages(struct connection *conn)
 {
 	int ret;
 	struct buffered_data *out;
+	bool started = false;
 
 	out = list_top(&conn->out_list, struct buffered_data, list);
 	if (out == NULL)
 		return true;
 
 	if (out->inhdr) {
+		started = !out->used;
 		ret = conn->funcs->write(conn, out->hdr.raw + out->used,
 					 sizeof(out->hdr) - out->used);
 		if (ret < 0)
-			return false;
+			goto err;
 
 		out->used += ret;
 		if (out->used < sizeof(out->hdr))
-			return true;
+			goto start;
 
 		out->inhdr = false;
 		out->used = 0;
 
 		/* Second write might block if non-zero. */
 		if (out->hdr.msg.len && !conn->domain)
-			return true;
+			goto start;
 	}
 
 	ret = conn->funcs->write(conn, out->buffer + out->used,
 				 out->hdr.msg.len - out->used);
 	if (ret < 0)
-		return false;
+		goto err;
 
 	out->used += ret;
 	if (out->used != out->hdr.msg.len)
-		return true;
+		goto start;
 
-	trace_io(conn, out, 1);
+	trace_io(conn, out, started ? "OUT" : "OUT(END)");
 
 	free_buffered_data(out, conn);
 
 	return true;
+
+ err:
+	trace_io(conn, out, "OUT(ERR)");
+	return false;
+
+ start:
+	if (started)
+		trace_io(conn, out, "OUT(START)");
+	return true;
 }
 
 static int undelay_request(void *_req)
@@ -2067,7 +2077,7 @@ static void process_message(struct connection *conn, struct buffered_data *in)
 
 	/* At least send_error() and send_reply() expects conn->in == in */
 	assert(conn->in == in);
-	trace_io(conn, in, 0);
+	trace_io(conn, in, "IN");
 
 	if ((unsigned int)type >= XS_TYPE_COUNT || !wire_funcs[type].func) {
 		eprintf("Client unknown operation %i", type);
-- 
2.35.3

Re: [PATCH] tools/xenstored: potentially split trace_io() out message

Posted by Julien Grall 4 months, 3 weeks ago

Hi Juergen,

On 23/11/2023 16:08, Juergen Gross wrote:
> Today write_messages() will call trace_io() after having written the
> complete message to the ring buffer or socket.
> 
> In case the message can't be written in one go, split it by writing
> one trace entry when starting the write and one when finishing it.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

With one remark below:

Reviewed-by: Julien Grall <jgrall@amazon.com>

> ---
> This patch is meant to go on top of the patch "tools/xenstored: remove
> \"-V\" command line option" in order to not lose any possible debug
> information.
> ---
>   tools/xenstored/core.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> index 9046b200bc..a14b240ed9 100644
> --- a/tools/xenstored/core.c
> +++ b/tools/xenstored/core.c
> @@ -121,7 +121,7 @@ void trace(const char *fmt, ...)
>   
>   static void trace_io(const struct connection *conn,
>   		     const struct buffered_data *data,
> -		     int out)
> +		     const char *type)

This change seems somewhat unrelated. It would be worth mentioning it in 
the commit message.

If you propose a new wording, I can update it while committing.

Cheers,

-- 
Julien Grall

Re: [PATCH] tools/xenstored: potentially split trace_io() out message

Posted by Juergen Gross 4 months, 3 weeks ago

On 11.12.23 20:11, Julien Grall wrote:
> Hi Juergen,
> 
> On 23/11/2023 16:08, Juergen Gross wrote:
>> Today write_messages() will call trace_io() after having written the
>> complete message to the ring buffer or socket.
>>
>> In case the message can't be written in one go, split it by writing
>> one trace entry when starting the write and one when finishing it.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> With one remark below:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
>> ---
>> This patch is meant to go on top of the patch "tools/xenstored: remove
>> \"-V\" command line option" in order to not lose any possible debug
>> information.
>> ---
>>   tools/xenstored/core.c | 30 ++++++++++++++++++++----------
>>   1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
>> index 9046b200bc..a14b240ed9 100644
>> --- a/tools/xenstored/core.c
>> +++ b/tools/xenstored/core.c
>> @@ -121,7 +121,7 @@ void trace(const char *fmt, ...)
>>   static void trace_io(const struct connection *conn,
>>                const struct buffered_data *data,
>> -             int out)
>> +             const char *type)
> 
> This change seems somewhat unrelated. It would be worth mentioning it in the 
> commit message.
> 
> If you propose a new wording, I can update it while committing.

What about:

In order to distinguish a complete OUT message from a split one, let the
caller of trace_io specify the prefix string ("IN", "OUT", "OUT(START)",
"OUT(END)") directly instead via an int.


Juergen

Re: [PATCH] tools/xenstored: potentially split trace_io() out message

Posted by Julien Grall 4 months, 3 weeks ago

Hi,

On 12/12/2023 06:26, Juergen Gross wrote:
> On 11.12.23 20:11, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 23/11/2023 16:08, Juergen Gross wrote:
>>> Today write_messages() will call trace_io() after having written the
>>> complete message to the ring buffer or socket.
>>>
>>> In case the message can't be written in one go, split it by writing
>>> one trace entry when starting the write and one when finishing it.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> With one remark below:
>>
>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>>
>>> ---
>>> This patch is meant to go on top of the patch "tools/xenstored: remove
>>> \"-V\" command line option" in order to not lose any possible debug
>>> information.
>>> ---
>>>   tools/xenstored/core.c | 30 ++++++++++++++++++++----------
>>>   1 file changed, 20 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
>>> index 9046b200bc..a14b240ed9 100644
>>> --- a/tools/xenstored/core.c
>>> +++ b/tools/xenstored/core.c
>>> @@ -121,7 +121,7 @@ void trace(const char *fmt, ...)
>>>   static void trace_io(const struct connection *conn,
>>>                const struct buffered_data *data,
>>> -             int out)
>>> +             const char *type)
>>
>> This change seems somewhat unrelated. It would be worth mentioning it 
>> in the commit message.
>>
>> If you propose a new wording, I can update it while committing.
> 
> What about:
> 
> In order to distinguish a complete OUT message from a split one, let the
> caller of trace_io specify the prefix string ("IN", "OUT", "OUT(START)",
> "OUT(END)") directly instead via an int.

Ah... I spotted the "OUT(END)" but somehow I didn't make the connection 
with the update of trace_io() prototype.

It is now committed.

Cheers,

-- 
Julien Grall