www.libssh2.org | Daily snapshots | Mailing list archive | Docs | Examples | github

Archive Index This month's Index

Subject: Re: [PATCH] Add support for "signal" message channel request

Re: [PATCH] Add support for "signal" message channel request

From: Pavel Strashkin <pavel.strashkin_at_gmail.com>
Date: Mon, 17 Oct 2011 20:05:26 -0700

> 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

the libssh2 team