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

[Paul Eggert <eggert@cs.ucla.edu>]

> +  /* 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.