Project

General

Profile

Actions
Copy link

Bug #6593

closed

[SDKs] arv-get should use the stdout FD directly, rather than opening /dev/stdout

Added by Brett Smith almost 9 years ago. Updated over 8 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Peter Amstutz
Category:
SDKs
Target version:
2015-08-05 sprint
Story points:
0.5

Description

A process can always write to fd 1, but it may not always be able to open /dev/stdout. In particular, if a user in an SSH session uses sudo to open a shell for a different non-root user, /dev/stdout will be a link to the user's original tty, which the new user probably won't be able to open.

The destination argument should default to None. The main body should catch that case and use sys.stdout as the output file in that case, rather than opening /dev/stdout. Update other checks as needed.

Subtasks 2 (0 open — 2 closed)

Task #6712: Review 6593-arv-get-stdoutResolvedPeter Amstutz07/13/2015Actions
Task #6711: FixResolvedPeter Amstutz07/13/2015Actions
Actions
Copy link
 #2

Updated by Brett Smith almost 9 years ago

  • Target version changed from 2015-08-19 sprint to 2015-08-05 sprint
Actions
Copy link
 #3

Updated by Peter Amstutz over 8 years ago

  • Assigned To set to Peter Amstutz
Actions
Copy link
 #4

Updated by Peter Amstutz over 8 years ago

  • Status changed from New to In Progress
Actions
Copy link
 #5

Updated by Brett Smith over 8 years ago

Reviewing ce12890

  • Please update where the command line option help strings refer to /dev/stdout. I think it would be fine to just change them to refer to plain "stdout."
  • Under the comment "# Turn on --progress by default", there's a condition args.destination != '/dev/stdout' that no longer works as intended.
  • This bug predates your branch, but I noticed it while reading: this block:
           if outfile and outfilename != '-':
             os.unlink(outfilename)

    would better avoid race conditions and other corner cases written as:
           if outfile and (outfile.fileno() > 2) and (not outfile.closed):
             os.unlink(outfile.name)

    (The old code might unlink the wrong file if the ^C comes in between reading a new outfilename and opening it.)

Thanks.

Actions
Copy link
 #6

Updated by Peter Amstutz over 8 years ago

Brett Smith wrote:

Reviewing ce12890

  • Please update where the command line option help strings refer to /dev/stdout. I think it would be fine to just change them to refer to plain "stdout."

Fixed.

  • Under the comment "# Turn on --progress by default", there's a condition args.destination != '/dev/stdout' that no longer works as intended.

Thanks for catching that, fixed.

  • This bug predates your branch, but I noticed it while reading: this block:
    [...]
    would better avoid race conditions and other corner cases written as:
    [...]
    (The old code might unlink the wrong file if the ^C comes in between reading a new outfilename and opening it.)

Done.

Thanks!

Actions
Copy link
 #7

Updated by Brett Smith over 8 years ago

Peter Amstutz wrote:

Brett Smith wrote:

  • This bug predates your branch, but I noticed it while reading: this block:
    [...]
    would better avoid race conditions and other corner cases written as:
    [...]
    (The old code might unlink the wrong file if the ^C comes in between reading a new outfilename and opening it.)

Done.

Sorry, but, uh, you did not copy and paste this correctly.

  • The unlink line should change its argument from outfilename to outfile.name.
  • outfile.closed is a boolean attribute, not a method. The condition currently in your branch will raise a TypeError. Remove the parentheses.

The branch is good to merge with these changes. Thanks.

Actions
Copy link
 #8

Updated by Peter Amstutz over 8 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:66af20886def83f6a20cc1e6587de00cbf2f8b59.

Actions
Copy link

Also available in: Atom PDF