Re: [PATCH] server: Add a ssh_send_keepalive() function

[Andreas Schneider <asn@xxxxxxxxxxxxxx>]

On Saturday 02 November 2013 22:33:42 Nicolas Viennot wrote:
> Implementation of a ssh_send_keepalive() function.
> Useful for keeping some client around as NATs may terminate connections.

Hi Nicolas,

did you check if/how the replies are handled? Also how do we find out if the 
client can handle it?

I would also see come code refactoring.
 
> Signed-off-by: Nicolas Viennot <nicolas@xxxxxxxxxxx>
> ---
>  include/libssh/server.h |  2 ++
>  src/server.c            | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/include/libssh/server.h b/include/libssh/server.h
> index dbffecd..9d095fe 100644
> --- a/include/libssh/server.h
> +++ b/include/libssh/server.h
> @@ -382,6 +382,8 @@ LIBSSH_API int ssh_channel_write_stderr(ssh_channel
> channel, const void *data, uint32_t len);
> 
> +LIBSSH_API int ssh_send_keepalive(ssh_session session);
> +
>  /* deprecated functions */
>  SSH_DEPRECATED LIBSSH_API int ssh_accept(ssh_session session);
>  SSH_DEPRECATED LIBSSH_API int channel_write_stderr(ssh_channel channel,
> diff --git a/src/server.c b/src/server.c
> index 0b62776..b31371f 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -1221,6 +1221,40 @@ int ssh_execute_message_callbacks(ssh_session
> session){ return SSH_OK;
>  }
> 
> +
> +int ssh_send_keepalive(ssh_session session)
> +{
> +  struct ssh_string_struct *req;
> +  int reply = 1;
> +  int rc = SSH_ERROR;
> +
> +  req = ssh_string_from_char("keepalive@xxxxxxxxxxx");
> +  if (req == NULL) {
> +    ssh_set_error_oom(session);
> +    goto out;
> +  }
> +
> +  if (buffer_add_u8(session->out_buffer, SSH2_MSG_GLOBAL_REQUEST) < 0 ||
> +      buffer_add_ssh_string(session->out_buffer, req) < 0 ||
> +      buffer_add_u8(session->out_buffer, reply == 0 ? 0 : 1) < 0) {
> +    ssh_set_error_oom(session);
> +    goto out;
> +  }

I would like this to be more like:

     rc = buffer_add_u8(session->out_buffer, SSH2_MSG_GLOBAL_REQUEST);
     if (rc < 0) {
         ssh_set_error_oom(session);
		  goto error;
     }

     req = ssh_string_from_char("keepalive@xxxxxxxxxxx");
     if (req == NULL) {
         ssh_set_error_oom(session);
         goto error;
     }

     rc =  buffer_add_ssh_string(session->out_buffer, req);
     string_free(req);
     if (rc < 0) {
         ssh_set_error_oom(session);
         goto error;
     }

	...

error:
     buffer_reinit(session->out_buffer);
     return SSH_ERROR;

This way the code is easier to debug.

gdb$ print rc

> +
> +  if (packet_send(session) == SSH_ERROR)
> +    goto out;

Same here.

     rc = packet_send(session);
     if (rc == SSH_ERROR) {
         goto out;
     }

Please always add brackets. We learned it the hard way in Samba. We had too 
many bugs cause of missing brackets.

> +
> +  ssh_handle_packets(session, 0);
> +
> +  SSH_LOG(SSH_LOG_PACKET, "Sent a keepalive");
> +  rc = SSH_OK;
> +
> +out:
> +  ssh_string_free(req);
> +  return rc;
> +}
> +
>  /** @} */
> 
>  /* vim: set ts=4 sw=4 et cindent: */