Bug #89208 for Net-SFTP-Foreign: error message from ssh connect failre not captured

Wed Oct 02 15:36:37 2013 tmetro [...] cpan.org - Ticket created

Subject:

error message from ssh connect failre not captured

Code like: my $host = 'doesnt-exist'; my $sftp = Net::SFTP::Foreign->new($host); $sftp->error and die "$host: failed to connect: " . $sftp->error, "\n"; will produce an error like: ssh: Could not resolve hostname doesnt-exist: Name or service not known doesnt-exist: failed to connect: Connection to remote server is broken Ideally your code should capture the STDERR output from the underlying ssh command and make it available through $sftp->error.

Wed Oct 02 15:48:45 2013 tmetro [...] cpan.org - Correspondence added

From:

tmetro [...] cpan.org

On Wed Oct 02 15:36:37 2013, TMETRO wrote: Show quoted text

> Ideally your code should capture the STDERR output from the underlying > ssh command and make it available through $sftp->error.

I just found the stderr_fh and stderr_discard constructor options. If you are confident in your error handling code, you should have stderr_discard => 1 be the default, and in a DEBUGGING section recommend users set it to stderr_discard => 0 for additional diagnostics.

Wed Oct 02 17:32:49 2013 salva [...] cpan.org - Correspondence added

On Wed Oct 02 15:48:45 2013, TMETRO wrote: Show quoted text

> On Wed Oct 02 15:36:37 2013, TMETRO wrote:

> > Ideally your code should capture the STDERR output from the > > underlying > > ssh command and make it available through $sftp->error.

It is not possible to capture stderr because the ssh slave process runs asynchronously. As you have already found, it is easy to redirect it to a file if you want to. Show quoted text

> > I just found the stderr_fh and stderr_discard constructor options. > > If you are confident in your error handling code, you should have > stderr_discard => 1 be the default, and in a DEBUGGING section > recommend users set it to stderr_discard => 0 for additional > diagnostics.

I don't agree with you, most ssh based utilities work the way the module does. In any case, it is too late to change the module API.

Wed Oct 02 17:32:49 2013 The RT System itself - Status changed from 'new' to 'open'

Wed Oct 02 17:32:50 2013 salva [...] cpan.org - Status changed from 'open' to 'rejected'

Thu Oct 03 12:50:05 2013 tmetro [...] cpan.org - Correspondence added

From:

tmetro [...] cpan.org

On Wed Oct 02 17:32:49 2013, SALVA wrote: Show quoted text

> It is not possible to capture stderr because the ssh slave process > runs asynchronously.

OK, that's fine. As long as your code is capturing adequate error information from the channel it is using to communicate with the ssh process. Show quoted text

> > I just found the stderr_fh and stderr_discard constructor options. > > > > If you are confident in your error handling code, you should have > > stderr_discard => 1 be the default, and in a DEBUGGING section > > recommend users set it to stderr_discard => 0 for additional > > diagnostics.

> > ...most ssh based utilities work the way the module does.

Such as? Show quoted text

> In any case, it is too late to change the module API.

I can't see suppressing STDERR by default to likely break many existing uses of this module, and yet it would be a benefit for new adopters of the module. You could be smart about the change and ignore stderr_discard => 1 if stderr_fh is set. Something like... (pseudo code): $err_fh = $stderr_fh || ($stderr_discard ? '/dev/null' : 'STDERR') As long as the interaction between $stderr_fh and $stderr_discard is documented, it should be fine.

Fri Oct 04 05:35:39 2013 salva [...] cpan.org - Correspondence added

Show quoted text

> > ...most ssh based utilities work the way the module does.

> > Such as?

Such as? It is difficult to find ones that DOESN'T work that way! Show quoted text

> > In any case, it is too late to change the module API.

> > I can't see suppressing STDERR by default to likely break many > existing uses of this module, and yet it would be a benefit for new > adopters of the module. > > You could be smart about the change and ignore stderr_discard => 1 if > stderr_fh is set. Something like... (pseudo code): > > $err_fh = $stderr_fh || ($stderr_discard ? '/dev/null' : 'STDERR') > > As long as the interaction between $stderr_fh and $stderr_discard is > documented, it should be fine.

I like the current default behaviour. It is the one that makes more sense to me because, as I said before, it is how most ssh-based utilities work and I find it quite useful. If you want to discard stderr, you only need to add an extra option into the constructor call. It is not a big hassle.