libssh2_at_git.stuge.se wrote:
> +#define sftp_attrsize(f) \
> + (4 + /* flags(4) */ \
> + ((f & LIBSSH2_SFTP_ATTR_SIZE)?8:0) + \
> + ((f & LIBSSH2_SFTP_ATTR_UIDGID)?8:0) + \
> + ((f & LIBSSH2_SFTP_ATTR_PERMISSIONS)?4:0) + \
> + ((f & LIBSSH2_SFTP_ATTR_ACMODTIME)?8:0)) /* atime + mtime as u32 */
I'd make sure to use (f) within the macro expansion.
> @@ -829,7 +815,7 @@ sftp_open(LIBSSH2_SFTP *sftp, const char *filename,
> /* packet_len(4) + packet_type(1) + request_id(4) + filename_len(4) +
> flags(4) */
> sftp->open_packet_len = filename_len + 13 +
> - (open_file? (4 + sftp_attrsize(&attrs)) : 0);
> + (open_file? (4 + sftp_attrsize(LIBSSH2_SFTP_ATTR_PERMISSIONS)) : 0);
Where does attrs come from? If it's not created in the function this
might desync libssh2 and server expectations. Same for other
occurences of sftp_attrsize() on constants, where the return value is
used in a packet. It just seems more defensive to me, and less
redundant, to use sftp_attrsize() on the actual value in the packet.
> +++ b/src/sftp.c
> @@ -831,6 +831,8 @@ sftp_open(LIBSSH2_SFTP *sftp, const char *filename,
> sftp->open_packet_len = filename_len + 13 +
> (open_file? (4 + sftp_attrsize(&attrs)) : 0);
>
> + /* surprise! this starts out with nothing sent */
> + sftp->open_packet_sent = 0;
> s = sftp->open_packet = LIBSSH2_ALLOC(session, sftp->open_packet_len);
> if (!sftp->open_packet) {
This looks redundant. I think there may be some more places where
there's code like this - it'd be nice to reduce rather than expand. :)
//Peter
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2010-04-28