From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.devel Subject: Re: wait_reading_process_ouput hangs in certain cases (w/ patches) Date: Wed, 22 Nov 2017 00:55:55 -0800 Organization: UCLA Computer Science Department Message-ID: <7550438b-9fd4-d374-e571-8bb16456cad5@cs.ucla.edu> References: <5150d198-8dd3-9cf4-5914-b7e945294452@binary-island.eu> <83tvy7s6wi.fsf@gnu.org> <83inemrqid.fsf@gnu.org> <398f8d17-b727-d5d6-4a31-772448c7ca0d@binary-island.eu> <56e722a6-95a4-0e42-185c-f26845d4f4bf@binary-island.eu> <21237e45-a353-92f9-01ec-7b51640d2031@cs.ucla.edu> <83vaickfu2.fsf@gnu.org> <83tvxwkexg.fsf@gnu.org> <03261534-6bf5-1a5d-915f-d3c55aaa35e9@binary-island.eu> <206ebefa-7583-f049-140c-c8fd041b0719@cs.ucla.edu> <709614e8-1937-07c1-f554-b453ed4f3d4a@binary-island.eu> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Trace: blaine.gmane.org 1511340976 855 195.159.176.226 (22 Nov 2017 08:56:16 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 22 Nov 2017 08:56:16 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 Cc: emacs-devel@gnu.org To: Matthias Dahl , Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Nov 22 09:56:10 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eHQpB-00080E-S6 for ged-emacs-devel@m.gmane.org; Wed, 22 Nov 2017 09:56:06 +0100 Original-Received: from localhost ([::1]:38420 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eHQpJ-00034Y-9B for ged-emacs-devel@m.gmane.org; Wed, 22 Nov 2017 03:56:13 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:39070) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eHQpC-00034I-Ms for emacs-devel@gnu.org; Wed, 22 Nov 2017 03:56:07 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eHQpB-00023O-NR for emacs-devel@gnu.org; Wed, 22 Nov 2017 03:56:06 -0500 Original-Received: from zimbra.cs.ucla.edu ([131.179.128.68]:47638) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eHQp5-0001wi-3b; Wed, 22 Nov 2017 03:55:59 -0500 Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 5346E1610E9; Wed, 22 Nov 2017 00:55:56 -0800 (PST) Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id Une93sMrgAQ6; Wed, 22 Nov 2017 00:55:55 -0800 (PST) Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 7E6201611F3; Wed, 22 Nov 2017 00:55:55 -0800 (PST) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id PR8PJMc3oQGY; Wed, 22 Nov 2017 00:55:55 -0800 (PST) Original-Received: from [192.168.1.9] (unknown [47.154.30.119]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id 59EE4160F81; Wed, 22 Nov 2017 00:55:55 -0800 (PST) In-Reply-To: <709614e8-1937-07c1-f554-b453ed4f3d4a@binary-island.eu> Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 131.179.128.68 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:220353 Archived-At: > + /* Don't count carryover as those bytes have already been count by > + a previous iteration. */ > + p->infd_num_bytes_read +=3D nbytes; > + This doesn't look right, as nbytes might be negative (indicating an error= ). > Subject: [PATCH 2/2] * src/process.c (wait_reading_process_output): Fix > wait_proc hang. Please start with a summary line that doesn't contain so much detail. <=3D= 50 chars=20 is good. No trailing period. "Fix wait_reading_process_output_hang", perh= aps. > + uintmax_t initial_wait_proc_num_bytes_read =3D (wait_proc) ? > + wait_proc->infd_num_byt= es_read : 0; Kind of a long name, no? Perhaps make it shorter, so that you can write=20 something like this: uintmax_t nbytes_read0 =3D wait_proc ? wait_proc->infd_num_bytes_read= : 0; Even better, shorten the member name too: "nbytes_read" is easier to read= than=20 "infd_num_bytes_read". > + /* Timers could have called `accept-process-output', thus re= ading the output Please limit the program to 80 columns. > + of wait_proc while we (in the worst case) wait endlessly = for it to become > + available later. So we need to check if data has been rea= d and break out > + early if that is so since our job has been fulfilled. */ > + if (wait_proc > + && wait_proc->infd_num_bytes_read !=3D initial_wait_proc= _num_bytes_read) > + { > + /* Make sure we don't overflow signed got_some_output. > + Calculating bytes read is modulo (UINTMAX_MAX + 1) an= d won't overflow. */ > + got_some_output =3D min(INT_MAX, (wait_proc->infd_num_by= tes_read > + - initial_wait_proc_num_= bytes_read)); Space before "(" in function calls. > + if (wait_proc > + && wait_proc->infd_num_bytes_read !=3D initial_wait_= proc_num_bytes_read) > + { > + /* Make sure we don't overflow signed got_some_outpu= t. > + Calculating bytes read is modulo (UINTMAX_MAX + 1= ) and won't overflow. */ > + got_some_output =3D min(INT_MAX, (wait_proc->infd_nu= m_bytes_read > + - initial_wait_proc_= num_bytes_read)); > + } It's annoying that the code (and the comment!) is duplicated. How about p= utting=20 it into a function? Also, there's nothing unusual about unsigned arithmet= ic=20 wrapping around; to me that (repeated) comment is almost as bad as writin= g "i++;=20 /* Add 1 to i. */". Perhaps that's just me, but at least the obvious com= ment=20 should not be duplicated. A function like this, perhaps? static int input_progress (struct Lisp_Process *wait_proc, uintmax_t nbytes_read0= ) { if (wait_proc) { /* This subtraction might wrap around; that's OK. */ uintmax_t progress =3D wait_proc->nbytes_read - nbytes_read0; if (progress !=3D 0) return min (progress, INT_MAX); } return -1; } and then the above chunks could be turned into something as simple as: got_some_output =3D input_progress (wait_proc, nbytes_read0); with maybe some other trivial changes to make this all work.