Re: [igt-dev] [PATCH i-g-t 2/6] igt_core: Split too long log lines when sending to runner with comms

[Petri Latvala <petri.latvala@intel.com>]

On Mon, Nov 07, 2022 at 05:40:06PM +0100, Kamil Konieczny wrote:
> Hi Petri,
> 
> On 2022-11-07 at 17:14:57 +0200, Petri Latvala wrote:
> > On Mon, Nov 07, 2022 at 03:16:18PM +0100, Kamil Konieczny wrote:
> > > Hi Petri,
> > > 
> > > On 2022-11-07 at 14:01:47 +0200, Petri Latvala wrote:
> > > > Especially with file dumps a single log packet could exceed the max
> > > > size of a UNIX datagram. Split too long log chunks instead.
> > > 
> > > imho this is needed when you check packet size in other patch,
> > > it's network layer decision to split or not packets, so on local
> > > machine no split happens (well, I may be wrong here as I do not
> > > know much about network communication). The other way around
> > > would be to collect packet up to sended size.
> > 
> > With SOCK_STREAM, sure, but runnercomms is using SOCK_DGRAM. Datagram
> > packets are full-or-nothing atomically.
> > 
> > 
> 
> Ah, ok, but then you risk getting your datagrams in out-of-order
> and when you will fix this you reimplement stream. If it is to
> be used on local machine maybe it should not happen.

AF_UNIX + SOCK_DGRAM guarantees same-order delivery.

-- 
Petri Latvala


> 
> Kamil
> 
> > > 
> > > > 
> > > > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > > > Cc: Arkadiusz Hiler <arek@hiler.eu>
> > > > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > > ---
> > > >  lib/igt_core.c | 19 ++++++++++++++++++-
> > > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > > index 3941c528..d4bef161 100644
> > > > --- a/lib/igt_core.c
> > > > +++ b/lib/igt_core.c
> > > > @@ -457,6 +457,23 @@ static void _igt_log_buffer_reset(void)
> > > >  	pthread_mutex_unlock(&log_buffer_mutex);
> > > >  }
> > > >  
> > > > +static void _log_to_runner_split(int stream, const char *str)
> > > > +{
> > > > +	size_t limit = 4096;
> > > > +	char *buf;
> > > > +
> > > > +	if (strlen(str) > limit) {
> > > > +		buf = calloc(limit + 1, 1);
> > > > +		strncpy(buf, str, limit);
> > > > +		send_to_runner(runnerpacket_log(stream, buf));
> > > > +		free(buf);
> > > > +
> > > > +		_log_to_runner_split(stream, str + limit);
> > > > +	} else {
> > > > +		send_to_runner(runnerpacket_log(stream, str));
> > > > +	}
> > > > +}
> > > > +
> > > 
> > > There are many places which calls send_to_runner, maybe it would
> > > be better to split it there ?
> > 
> > That would complicate send_to_runner a bit too much to my
> > taste. send_to_runner() is for all packet types, not just for log
> > packets. It would have to check for packet type log, extract the data
> > from that packet, and then create new ones...
> > 
> > > Or use _log_to_runner instead of send_to_runner ?
> > > Btw a while loop would be better than recursive call.
> > 
> > Yeah, you're right. I'll change that to a while loop.
> > 
> > 
> > -- 
> > Petri Latvala
> > 
> > 
> > > 
> > > Kamil
> > > 
> > > >  __attribute__((format(printf, 2, 3)))
> > > >  static void _log_line_fprintf(FILE* stream, const char *format, ...)
> > > >  {
> > > > @@ -467,7 +484,7 @@ static void _log_line_fprintf(FILE* stream, const char *format, ...)
> > > >  
> > > >  	if (runner_connected()) {
> > > >  		vasprintf(&str, format, ap);
> > > > -		send_to_runner(runnerpacket_log(fileno(stream), str));
> > > > +		_log_to_runner_split(fileno(stream), str);
> > > >  		free(str);
> > > >  	} else {
> > > >  		vfprintf(stream, format, ap);
> > > > -- 
> > > > 2.30.2
> > > >