From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1CE2D10E440 for ; Mon, 7 Nov 2022 16:45:06 +0000 (UTC) Date: Mon, 7 Nov 2022 18:45:08 +0200 From: Petri Latvala To: Kamil Konieczny , igt-dev@lists.freedesktop.org, Arkadiusz Hiler Message-ID: References: <20221107120151.2365523-1-petri.latvala@intel.com> <20221107120151.2365523-2-petri.latvala@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [igt-dev] [PATCH i-g-t 2/6] igt_core: Split too long log lines when sending to runner with comms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 > > > > Cc: Arkadiusz Hiler > > > > Cc: Kamil Konieczny > > > > --- > > > > 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 > > > >