Page MenuHomePhabricator

Difference between sftp_new() and sftp_init() is not clear
Closed, ResolvedPublic

Description

The API documentation for sftp_new() at http://api.libssh.org/master/group__libssh__sftp.html#gae84eeb5a1af9d49db10f17a060151e97 says

Start a new sftp session.

The documentation for sftp_init() at http://api.libssh.org/master/group__libssh__sftp.html#gab8ec37b2cb85c9bb47341683f43f0841 says

Initialize the sftp session with the server.

So the documentation suggests that communication with the server only happens at sftp_init() when in fact, already sftp_new() allocates the channel and communicates with the server and when the sftp subsystem is not enabled in the sshd server, it will fail with error message

Channel request subsystem failed

Also the documentation at http://api.libssh.org/master/libssh_tutor_sftp.html shows code

sftp = sftp_new(session);
if (sftp == NULL)
{
  fprintf(stderr, "Error allocating SFTP session: %s\n",
          ssh_get_error(session));
  return SSH_ERROR;
}

where the error message Error allocating suggests that sftp_new() is merely about allocating the memory for the structure locally, like ssh_new() does.

What is the difference between the two? Why are there two separate calls at all? Shouldn't sftp_new() just allocate and only sftp_init() do any communication on the network?

Event Timeline

adelton created this task.Mar 14 2019, 2:15 PM

The behaviour was observed on Fedora 29 with libssh-0.8.7-1.fc29.x86_64.

asn added a project: Restricted Project.Apr 29 2019, 10:10 AM
Jakuje added a subscriber: Jakuje.Sep 13 2019, 2:47 PM

The file doc/sftp.dox contains the following information (not sure whether it is rendered somewhere on the web):

The function sftp_new() creates a new SFTP session. The function sftp_init()
initializes it. The function sftp_free() deletes it.

This gives basic overview, but I agree that the above documentation is not sufficient.

From my understanding, the sftp_new() takes existing SSH session, allocates SFTP session and opens the SFTP channel.

On the other hand, sftp_init() already takes the SFTP session and already sends initializing SFTP messages (see the protocol initialization in the SFTP RFC draft). From here, probably the names.

I am not the author of this code, so I can probably not comment on the reason why these two separated functions were created, when neither can live without the other, but I can at least try to clarify the documentation so it it less confusing. What would you say to this improved and extended wording in the proposed patch:

https://gitlab.com/jjelen/libssh-mirror/commit/8184103d6b80e6fe50daf06a002f5118bfa4194c

aris added a subscriber: aris.Sep 13 2019, 6:37 PM

That code is awfully old (probably 2004-2005). The idea was probably separating the SSH part and the SFTP parts. Today it makes little sense. Also both calls are synchronous and blocking. Your proposed patch describes well what it does today. Given the time I'd improve the actual implementation and move everything that handles network into sftp_init() and leave sftp_new() to memory allocation only like it's standard in the rest of the library.

I like the proposed wording, @Jakuje.

If the semantics get changed later, the documentation will change with it. But I'd prefer not waiting for the refactoring and just update the description now, as the first step.

aris added a comment.Sep 15 2019, 9:11 PM
In T137#2665, @adelton wrote:

I like the proposed wording, @Jakuje.
If the semantics get changed later, the documentation will change with it. But I'd prefer not waiting for the refactoring and just update the description now, as the first step.

I Agree.

Jakuje closed this task as Resolved.Sep 16 2019, 12:56 PM
Jakuje claimed this task.