-
Notifications
You must be signed in to change notification settings - Fork 528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement posix-rename@openssh.com #1386
base: master
Are you sure you want to change the base?
Conversation
d030545
to
a27185c
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of a new API, what about going with size_t
sizes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the two new .3
files to doc/Makefile.am
would likely fix the distrocheck job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I'll do that.
This commit adds a new function, libssh2_sftp_posix_rename_ex, which implements the posix-rename@openssh.com 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.
a27185c
to
d1061da
Compare
This commit adds a new function, libssh2_sftp_posix_rename_ex, which implements the posix-rename@openssh.com 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.