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 14:27:14 -0700

* remove changes for "_libssh2_error"
* fix "memeory" typo
* add missing signals to "valid_signals" array
* use ARRAY_SIZE instead of sizeof to get number of elements in
"valid_signals" array
* make "valid_signals" array to be super constant
* fix comment header for "libssh2_channel_signal" function

> Since this list is search linearly, you could make it ever so slightly faster
> by sorting it in decreasing order or usage frequency. That would be hard to
> determine, but 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.

2011/10/17 Dan Fandrich <dan_at_coneharvesters.com>:
> On Mon, Oct 17, 2011 at 01:01:03PM -0700, Pavel Strashkin wrote:
>> + * _libssh2_channel_signal
>> + *
>> + * Deliver a signal to a shell/program/subsystem.
>> + */
>> +static int _libssh2_channel_signal(LIBSSH2_CHANNEL *channel,
>> +                          const char *signame, size_t signame_len)
>> +{
>> +    LIBSSH2_SESSION *session = channel->session;
>> +    static const char *valid_signals[] = {
>
> This should be static const char * const valid_signals[] to make the array
> itself constant.
>
>> +        "ABRT",
>> +        "ALRM",
>> +        "FPE",
>> +        "HUP",
>> +        "ILL",
>> +        "INT",
>> +        "KILL",
>> +        "PIPE",
>> +        "QUIT",
>> +        "SEGV",
>
> Since this list is search linearly, you could make it ever so slightly faster
> by sorting it in decreasing order or usage frequency. That would be hard to
> determine, but I'd put signal like QUIT and HUP before ones like FPE. It's a
> microoptimization, but it comes for free :)
>
>> +        0
>> +    };
>> +
>> +    int signal_is_valid = 0;
>> +    int i = 0;
>> +
>> +    for (i = 0; i < sizeof(valid_signals); i++) {
>
> sizeof is wrong here. It should be ARRAY_SIZE(valid_signals).
>
>> +        if (strcmp(signame, valid_signals[i]) == 0) {
>> +            signal_is_valid = 1;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (signal_is_valid == 0) {
>> +        return _libssh2_error(session, LIBSSH2_ERROR_INVALID_SIGNAL_NAME,
>> +            "The signal name '%s' is invalid. Must be one of the following: "
>> +            "ABRT, ALRM, FPE, HUP, ILL, INT, KILL, PIPE, QUIT, SEGV, TERM, "
>> +            "USR1, USR2.",
>
> This list doesn't match the list in valid_signals[].
>
>> +            signame
>> +        );
>> +    }
>> +
>> +    if (channel->signal_state == libssh2_NB_state_idle) {
>> +        /*
>> +         * packet_type(1) + channel_id(4) + "signal"(6) + want_reply(1) +
>> +         * signame(signame_len)
>> +         */
>> +        channel->signal_packet_len = 1 + 4 + 6 + 1 + signame_len;
>> +
>> +        /* Zero the whole thing out */
>> +        memset(&channel->signal_packet_requirev_state, 0,
>> +               sizeof(channel->signal_packet_requirev_state));
>> +
>> +        _libssh2_debug(session, LIBSSH2_TRACE_CONN,
>> +            "Sending signal '%s' to channel %lu/%lu",
>> +            signame,
>> +            channel->local.id,
>> +            channel->remote.id);
>> +
>> +        unsigned char *s = channel->signal_packet =
>> +            LIBSSH2_ALLOC(session, channel->signal_packet_len);
>> +        if (!channel->signal_packet) {
>> +            return _libssh2_error(session, LIBSSH2_ERROR_ALLOC,
>> +                                  "Unable to allocate memeory "
>
> Typo: memeory
>
>>>> Dan
> _______________________________________________
> 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-17

the libssh2 team