Pavel Strashkin wrote:
> > I'd put signal like QUIT and HUP before ones like FPE. It's a
> > microoptimization, but it comes for free :)
>
> I'd prefer to keep it in the same order as in RFC, doc file and
> error string, if you don't mind.
I agree with having the list in a familiar order.
> diff --git a/docs/Makefile.am b/docs/Makefile.am
Please create a commit, and let git generate a patch. This way you
can also get feedback on the commit message, and you save a lot of
time for the code maintainers, and finally you will automatically
get credited correctly in the history.
After you have made a commit you can still make changes to the
commit, using git commit --amend. See also Pro Git about rewriting
history.
> +++ b/docs/Makefile.am
> @@ -25,8 +25,9 @@ dist_man_MANS = \
> libssh2_channel_forward_listen.3 \
> libssh2_channel_forward_listen_ex.3 \
> libssh2_channel_free.3 \
> - libssh2_channel_get_exit_signal.3 \
> + libssh2_channel_signal.3 \
> libssh2_channel_get_exit_status.3 \
> + libssh2_channel_get_exit_signal.3 \
Please avoid any unrelated changes in your commits. I agree with the
change to sort these correctly, but that should be done in a separate
commit. Please create multiple commits in a local branch and have git
generate (or send) patches, using git format-patch, or git
send-email.
> +++ b/docs/libssh2_channel_signal.3
..
> +.SH RETURN VALUE
> +Numeric error code corresponding to the the Error Code constants.
..
> +++ b/include/libssh2.h
> @@ -404,6 +404,7 @@ typedef struct _LIBSSH2_POLLFD {
> #define LIBSSH2_ERROR_SOCKET_RECV -43
> #define LIBSSH2_ERROR_ENCRYPT -44
> #define LIBSSH2_ERROR_BAD_SOCKET -45
> +#define LIBSSH2_ERROR_INVALID_SIGNAL_NAME -46
Is this new constant documented somewhere too?
> @@ -748,6 +749,10 @@ LIBSSH2_API int libssh2_channel_flush_ex(LIBSSH2_CHANNEL *channel,
> #define libssh2_channel_flush_stderr(channel) \
> libssh2_channel_flush_ex((channel), SSH_EXTENDED_DATA_STDERR)
>
> +LIBSSH2_API int libssh2_channel_signal(LIBSSH2_CHANNEL* channel,
> + const char *signame,
> + size_t signame_len);
> +
> LIBSSH2_API int libssh2_channel_get_exit_status(LIBSSH2_CHANNEL* channel);
> LIBSSH2_API int libssh2_channel_get_exit_signal(LIBSSH2_CHANNEL* channel,
> char **exitsignal,
Sort order.. It's in logical order but perhaps alphabetical is
better? Dunno.
> +++ b/src/channel.c
..
> +static int _libssh2_channel_signal(LIBSSH2_CHANNEL *channel,
> + const char *signame, size_t signame_len)
I disagree with this interface. First I would remove signame_len,
noone wants to deal with that. Second, I would rather use a uint8_t
or uint16_t parameter for the signal, and define all posible signals
in an enum in the header file. Again, noone wants to deal with
strings.
> + static const char* const valid_signals[] = {
> + "ABRT",
> + "ALRM",
..
> + if (signal_is_valid == 0) {
> + return _libssh2_error(session, LIBSSH2_ERROR_INVALID_SIGNAL_NAME,
> + "The signal name is invalid. Must be one of the following: "
> + "ABRT, ALRM, FPE, HUP, ILL, INT, KILL, PIPE, QUIT, SEGV, TERM, "
> + "USR1, USR2."
> + );
A style note; the above closing parenthesis should probably be at the
end of the line before.
But more importantly, I don't like that the list of these signals
appears in two places. I would like the error message to somehow
cleverly use the canonical list of signals that the code also uses.
//Peter
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2011-10-18