Subject: Re: timeouts in libssh2 blocking-mode

Re: timeouts in libssh2 blocking-mode

From: Peter Stuge <peter_at_stuge.se>
Date: Tue, 3 May 2011 01:59:37 +0200

Matt Lilley wrote:
> Ok, that seems reasonable to me. I've attached a new patch that does this
> (I think), and still passes my test cases.

I'll let you and Daniel sort the implementation details. Just one
comment:

> +++ b/src/session.h
> @@ -51,17 +51,20 @@
> function.
>
> */
> -#define BLOCK_ADJUST(rc,sess,x) \
> +#define BLOCK_ADJUST(rc,sess,x) { \
> + time_t entry_time = time (NULL); \
> do { \
> rc = x; \
> /* the order of the check below is important to properly deal with the
> case when the 'sess' is freed */ \
> if((rc != LIBSSH2_ERROR_EAGAIN) || !sess->api_block_mode) \
> break; \
> - rc = _libssh2_wait_socket(sess); \
> + rc = _libssh2_wait_socket(sess, entry_time); \
> if(rc) \
> break; \
> - } while(1)
> + } while(1); \
> +}
> +

No. Using do {} while() must not change (you add braces) since it
allows the macro to successfully be used as a function in all
circumstances in C code. In this case you would add an outer
do {} while() as such:

#define BLOCK_ADJUST(rc,sess,x) { \
  do { \
    time_t entry_time ... \
    do { \
       ...
    } while(1);
  } while(0)

Also, please make sure to maintain code style (whitespace changes) in
the code that you work on. Thanks!

Btw, it doesn't make sense to have while(1) immediately preceded by a
conditional break IMO. Might as well fix that too while touching
this:

  .. while(!rc);

> @@ -69,7 +72,8 @@
> * immediately. If the API is blocking and we get a NULL we check the errno
> * and *only* if that is EAGAIN we loop and wait for socket action.
> */
> -#define BLOCK_ADJUST_ERRNO(ptr,sess,x) \
> +#define BLOCK_ADJUST_ERRNO(ptr,sess,x) { \

Same comment as above.

//Peter
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2011-05-03