> Is this new constant documented somewhere too?
Are those all constants documented somewhere? :)
> Sort order.. It's in logical order but perhaps alphabetical is
> better? Dunno.
Actually i tried to don't touch any order of definitions, i just added
a function before some mark
and keep it in sync everywhere in the code (including that Makefile.am
which you saw at the beginning of patch).
> 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.
I'd like to keep API in the same way as RFC describes it. I don't know
why they choose strings for signals instead of
numbers. There was the reason. They define concrete signal names. And
also libssh2_channel_signal(..., "QUIT") looks better and more
readable than libssh2_channel_signal(..., 7). I believe 99%
programmers remember only than 9 == TERM and 15 == KILL.
> 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.
There is no easy way to join all valid_signals elements. The price of
this duplicate is really low.
2011/10/17 Peter Stuge <peter_at_stuge.se>:
> 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
>
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2011-10-18