emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: wait_reading_process_ouput hangs in certain cases (w/ patches)


From: Paul Eggert
Subject: Re: wait_reading_process_ouput hangs in certain cases (w/ patches)
Date: Wed, 22 Nov 2017 00:55:55 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

+  /* Don't count carryover as those bytes have already been count by
+     a previous iteration.  */
+  p->infd_num_bytes_read += 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. <=50 chars is good. No trailing period. "Fix wait_reading_process_output_hang", perhaps.

+  uintmax_t initial_wait_proc_num_bytes_read = (wait_proc) ?
+                                               wait_proc->infd_num_bytes_read 
: 0;

Kind of a long name, no? Perhaps make it shorter, so that you can write something like this:

   uintmax_t nbytes_read0 = wait_proc ? wait_proc->infd_num_bytes_read : 0;

Even better, shorten the member name too: "nbytes_read" is easier to read than "infd_num_bytes_read".

+          /* Timers could have called `accept-process-output', thus reading 
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 read and 
break out
+             early if that is so since our job has been fulfilled. */
+          if (wait_proc
+              && wait_proc->infd_num_bytes_read != 
initial_wait_proc_num_bytes_read)
+            {
+              /* Make sure we don't overflow signed got_some_output.
+                 Calculating bytes read is modulo (UINTMAX_MAX + 1) and won't 
overflow. */
+              got_some_output = min(INT_MAX, (wait_proc->infd_num_bytes_read
+                                              - 
initial_wait_proc_num_bytes_read));


Space before "(" in function calls.
+              if (wait_proc
+                  && wait_proc->infd_num_bytes_read != 
initial_wait_proc_num_bytes_read)
+                {
+                  /* Make sure we don't overflow signed got_some_output.
+                     Calculating bytes read is modulo (UINTMAX_MAX + 1) and 
won't overflow. */
+                  got_some_output = min(INT_MAX, 
(wait_proc->infd_num_bytes_read
+                                                  - 
initial_wait_proc_num_bytes_read));
+                }

It's annoying that the code (and the comment!) is duplicated. How about putting it into a function? Also, there's nothing unusual about unsigned arithmetic wrapping around; to me that (repeated) comment is almost as bad as writing "i++; /* Add 1 to i. */". Perhaps that's just me, but at least the obvious comment 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 = wait_proc->nbytes_read - nbytes_read0;
        if (progress != 0)
          return min (progress, INT_MAX);
      }
    return -1;
  }

and then the above chunks could be turned into something as simple as:

  got_some_output = input_progress (wait_proc, nbytes_read0);

with maybe some other trivial changes to make this all work.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]