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

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> If read_pipe crashes then the caller can inspect the error and handle
> it appropriately.
>
> Signed-off-by: Lars Schneider 
> ---
>  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?

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

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

[Lars Schneider]

On 09 Sep 2015, at 18:00, Junio C Hamano  wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> If read_pipe crashes then the caller can inspect the error and handle
>> it appropriately.
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[larsxschneider]

From: Lars Schneider 

If read_pipe crashes then the caller can inspect the error and handle
it appropriately.

Signed-off-by: Lars Schneider 
---
 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()))
 
 return val
 
-- 
2.5.1

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