> 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 <[email protected]>: > 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
