Implement [email protected] #1386
Conversation
MichaelBuckley
force-pushed
the
posix-rename
branch
2 times, most recently
from
d030545
to
a27185c
Compare
docs/libssh2_sftp_unlink_ex.3
Outdated
| @@ -9,7 +9,7 @@ libssh2_sftp_unlink_ex - unlink an SFTP file | |||
| #include <libssh2_sftp.h> | |||
|
|
|||
| int | |||
| libssh2_sftp_unlink_ex(LIBSSH2_SFTP *sftp, const char *filename, unsigned int filename_len); | |||
| libssh2_sftp_unlink_ex(LIBSSH2_SFTP *sftp, const char *filename, size_t filename_len); | |||
There was a problem hiding this comment.
We'd want that definitely, but that looks like an ABI break. We can either postpone it to a future "ABI break" release or add libssh2_sftp_unlink_ex2 or similar.
There was a problem hiding this comment.
I'm honestly not sure why this change was part of this commit in the Panic repo. I can revert it so it doesn't break ABI.
There was a problem hiding this comment.
Yeah, I agree to keep it separate. It's a good change, we should probably keep track of it in docs/TODO's At next SONAME bump section to remember.
| const char *source_filename, | ||
| unsigned int srouce_filename_len, | ||
| const char *dest_filename, | ||
| unsigned int dest_filename_len); |
There was a problem hiding this comment.
Speaking of a new API, what about going with size_t sizes?
There was a problem hiding this comment.
Sounds good to me.
| libssh2_sftp_posix_rename_ex((sftp), (sourcefile), \ | ||
| (unsigned int)strlen(sourcefile), \ | ||
| (destfile), (unsigned int)strlen(destfile)) | ||
|
|
There was a problem hiding this comment.
Would it make sense to publish the non-_ex flavour exclusively?
It seems to me that existing _ex variants were added because the initial implementation missed to provide the size arguments. Speaking of a new API, we provide them right away in the non-_ex API. Making a separate _ex one superfluous.
There was a problem hiding this comment.
In theory, I agree with this.
In practice, however, at Panic, all our code calls the non-_ex version because we don't need the flexability of setting the string size, and we don't want to call strlen() all the time in our code.
I suspect 99% of users don't want to have to call a function where they provide string lengths, and would be happier calling a function that just takes NULL-terminated strings.
There was a problem hiding this comment.
Okay, we can stick to this to avoid confusion.
include/libssh2_sftp.h
Outdated
| LIBSSH2_API int libssh2_sftp_unlink_ex(LIBSSH2_SFTP *sftp, | ||
| const char *filename, | ||
| unsigned int filename_len); | ||
| size_t filename_len); |
src/sftp.c
Outdated
| @@ -2863,7 +2863,7 @@ static int sftp_unlink(LIBSSH2_SFTP *sftp, const char *filename, | |||
| */ | |||
| LIBSSH2_API int | |||
| libssh2_sftp_unlink_ex(LIBSSH2_SFTP *sftp, const char *filename, | |||
| unsigned int filename_len) | |||
| size_t filename_len) | |||
| received on the socket, or an SFTP operation caused an errorcode to | ||
| be returned by the server. | ||
| .SH SEE ALSO | ||
| .BR libssh2_sftp_init(3) |
There was a problem hiding this comment.
Adding the two new .3 files to doc/Makefile.am would likely fix the distrocheck job.
There was a problem hiding this comment.
Thank you! I'll do that.
This commit adds a new function, libssh2_sftp_posix_rename_ex, which implements the [email protected] extension. If the server does not support this extension, the function will return LIBSSH2_FX_OP_UNSUPPORTED and it will be up to the user to recover, possibly by calling libssh2_sftp_rename.
MichaelBuckley
force-pushed
the
posix-rename
branch
from
a27185c
to
d1061da
Compare
This commit adds a new function, libssh2_sftp_posix_rename_ex, which implements the [email protected] extension.
If the server does not support this extension, the function will return LIBSSH2_FX_OP_UNSUPPORTED and it will be up to the user to recover, possibly by calling libssh2_sftp_rename.