Re: call-process should not block process filters from running

[sbaugh@catern.com]

Po Lu <luangruo@yahoo.com> writes:

> sbaugh@catern.com writes:
>
>>  /* Clean up files, file descriptors and processes created by Fcall_process.  */
>
> This is a stray comment.
>
>> +struct synch_process {
>> +  /* If nonzero, a process-ID that has not been reaped.  */
>> +  pid_t pid;
>> +  /* If a string, the name of a temp file that has not been removed.  */
>> +  Lisp_Object tempfile;
>> +  int* callproc_fd;
>> +  Lisp_Object buffer;
>> +};
>
> Please place the opening brace of this structure definition on a new
> line, fill the comments above each field, and write:
>
>   int *callproc_fd;
>
> instead of
>
>   int* callproc_fd;
>
> ISTM that this field could be named `fd' instead.
>
>> +  struct synch_process synch_process = {
>> +    .pid = 0,
>> +    .tempfile = make_fixnum (0),
>> +    .callproc_fd = callproc_fd,
>> +    .buffer = Fcurrent_buffer (),
>> +  };
>
> Why not declare `sync_process' earlier?  The code looks better that way.
>
>> +	      wait_reading_process_output(-1, -1, 0, display_on_the_fly,
>> +					  NULL, NULL, 0);
>> +	      swallow_events(display_on_the_fly);
>
> Please place a space between the function identifier and its parameter
> list.
>
>>  	      if (display_on_the_fly)
>>  		break;
>>  	    }
>> @@ -904,7 +897,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
>>  
>>    /* Don't kill any children that the subprocess may have left behind
>>       when exiting.  */
>> -  synch_process_pid = 0;
>> +  synch_process.pid = 0;
>>  
>>    SAFE_FREE_UNBIND_TO (count, Qnil);
>>  
>> @@ -2035,11 +2028,6 @@ syms_of_callproc (void)
>>  #endif
>>    staticpro (&Vtemp_file_name_pattern);
>>  
>> -#ifdef MSDOS
>> -  synch_process_tempfile = make_fixnum (0);
>> -  staticpro (&synch_process_tempfile);
>> -#endif
>
> Please make your modifications conditional on _not_ building the MS-DOS
> port: as it is a single process ``operating system'', Emacs does not run
> while a subprocess is running, so these changes are unnecessary.

Thanks, I will make all these changes in next iteration of the patch.

> I've also pointed out a general issue with this change: when a timer or
> selection converter calls `call-process', it does not expect more timers
> to run from within.  Assume that a timer periodically calls `rm
> /var/run/foo', and it takes a few seconds for `/bin/rm' to page in,
> enough for that timer to run again.  Now, one of the calls to `rm' will
> fail and signal an error.

AFAIK, timers don't run concurrently with themselves.  If a timer
function is running, a second instance of that function won't run until
the first one is finished.