Re: [PATCH v1 1/2] git-p4: print stderr if P4 read_pipe operation fails

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

 



On 09 Sep 2015, at 18:00, Junio C Hamano <gitster@xxxxxxxxx> wrote:

> larsxschneider@xxxxxxxxx writes:
> 
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>> 
>> If read_pipe crashes then the caller can inspect the error and handle
>> it appropriately.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@xxxxxxxxx>
>> ---
>> git-p4.py | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 073f87b..36a4bcb 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -134,11 +134,11 @@ def read_pipe(c, ignore_error=False):
>>         sys.stderr.write('Reading pipe: %s\n' % str(c))
>> 
>>     expand = isinstance(c,basestring)
>> -    p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
>> +    p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)
>>     pipe = p.stdout
>>     val = pipe.read()
>>     if p.wait() and not ignore_error:
>> -        die('Command failed: %s' % str(c))
>> +        die('Command failed: %s\nError: %s' % (str(c), p.stderr.read()))
> 
> I don't know enough about the callers of this helper function to
> tell offhand if that is an issue, but this looks unsafe depending on
> what the process on the other side of these pipes are doing.
> 
> If it attempts to spew a lot on its standard error stream first and
> then write some to its standard output, I would not be surprised it
> would get stuck waiting for us to read and drain its standard error
> before it can proceed to write to its standard output, and in the
> meantime we would be waiting for it to say something on its standard
> output, no?
> 
You are right. I will use the “communicate” function here as recommended in the Python docs:
https://docs.python.org/2/library/subprocess.html#subprocess.Popen.wait

Thanks!

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]