Re: [Tarantool-patches] [PATCH 1/1] iproto: show real port in logs and box.info.listen

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch, LGTM with some comments nits below.

Sergos.


On 12 ноя 23:03, Vladislav Shpilevoy wrote:
> Box.cfg{listen = 0} automatically chooses a port. But it was
> impossible to learn a real port the instance is bound to.
                ^^^^^ obtain?
> 
> An ability to see a real port may help to make test-run more
> robust, because it won't depend on which ports are free, and
> won't need to pre-choose them in advance.
> 
> Now box.info.listen shows a real address, or nil when listen is
> turned off. Also a real address is logged instead of the dummy
> 0-port one.
> 
> Closes #4620
> 
> @TarantoolBot document
> Title: box.info.listen - real address
> 
> New value in box.info - listen. It is a real address to which the
> instance was bound. For example, if box.cfg.listen was set with
> a zero port, box.info.listen will show a real port. The address
> is stored as a string:
> 
>     - unix/:<path> for UNIX domain sockets;
>     - <ip>:<port> for IPv4;
>     - [ip]:<port> for IPv6.
> 
> If the instance does not listen anything, box.info.listen is nil.
> ---
> Branch: https://github.com/tarantool/tarantool/tree/gerold103/gh-4620-show-listen-port
> Issue: https://github.com/tarantool/tarantool/issues/4620
> 
>  src/box/iproto.cc    | 31 +++++++++++++++++++++++++++++--
>  src/box/iproto.h     |  7 +++++++
>  src/box/lua/info.c   |  9 +++++++++
>  src/lib/core/evio.c  |  8 ++++++++
>  test/box/info.result |  1 +
>  5 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index 34c8f469a..522c066be 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -129,6 +129,23 @@ unsigned iproto_readahead = 16320;
>  /* The maximal number of iproto messages in fly. */
>  static int iproto_msg_max = IPROTO_MSG_MAX_MIN;
>  
> +/**
> + * Address the iproto listens for, stored in TX
> + * thread. Is kept in TX to be showed in box.info.
                                  shown, since V3?
> + */
> +static struct sockaddr_storage iproto_bound_address_storage;
> +/** 0 means that no address is listened. */
> +static socklen_t iproto_bound_address_len;
> +
> +const char *
> +iproto_bound_address(void)
> +{
> +	if (iproto_bound_address_len == 0)
> +		return NULL;
> +	return sio_strfaddr((struct sockaddr *) &iproto_bound_address_storage,
> +			    iproto_bound_address_len);
> +}
> +
>  /**
>   * How big is a buffer which needs to be shrunk before
>   * it is put back into buffer cache.
> @@ -2081,8 +2098,14 @@ struct iproto_cfg_msg: public cbus_call_msg
>  	/** Operation to execute in iproto thread. */
>  	enum iproto_cfg_op op;
>  	union {
> -		/** New URI to bind to. */
> -		const char *uri;
> +		struct {
> +			/** New URI to bind to. */
> +			const char *uri;
> +			/** Result address. */
> +			struct sockaddr_storage addr;
> +			/** Address length. */
> +			socklen_t addrlen;
> +		};
>  
>  		/** New iproto max message count. */
>  		int iproto_msg_max;
> @@ -2118,6 +2141,8 @@ iproto_do_cfg_f(struct cbus_call_msg *m)
>  			    (evio_service_bind(&binary, cfg_msg->uri) != 0 ||
>  			     evio_service_listen(&binary) != 0))
>  				diag_raise();
> +			cfg_msg->addrlen = binary.addr_len;
> +			cfg_msg->addr = binary.addrstorage;
>  			break;
>  		default:
>  			unreachable();
> @@ -2143,6 +2168,8 @@ iproto_listen(const char *uri)
>  	iproto_cfg_msg_create(&cfg_msg, IPROTO_CFG_LISTEN);
>  	cfg_msg.uri = uri;
>  	iproto_do_cfg(&cfg_msg);
> +	iproto_bound_address_storage = cfg_msg.addr;
> +	iproto_bound_address_len = cfg_msg.addrlen;
>  }
>  
>  size_t
> diff --git a/src/box/iproto.h b/src/box/iproto.h
> index edb24a7ed..201e09df5 100644
> --- a/src/box/iproto.h
> +++ b/src/box/iproto.h
> @@ -80,6 +80,13 @@ iproto_request_count(void);
>  void
>  iproto_reset_stat(void);
>  
> +/**
> + * String representation of the address served by
> + * iproto. To be showed in box.info.
                    shown
> + */
> +const char *
> +iproto_bound_address(void);
> +
>  #if defined(__cplusplus)
>  } /* extern "C" */
>  
> diff --git a/src/box/lua/info.c b/src/box/lua/info.c
> index 55382fd77..98579e8df 100644
> --- a/src/box/lua/info.c
> +++ b/src/box/lua/info.c
> @@ -485,6 +485,14 @@ lbox_info_vinyl(struct lua_State *L)
>  	return 1;
>  }
>  
> +static int
> +lbox_info_listen(struct lua_State *L)
> +{
> +	/* NULL is ok, no need to check. */
> +	lua_pushstring(L, iproto_bound_address());
> +	return 1;
> +}
> +
>  static const struct luaL_Reg lbox_info_dynamic_meta[] = {
>  	{"id", lbox_info_id},
>  	{"uuid", lbox_info_uuid},
> @@ -500,6 +508,7 @@ static const struct luaL_Reg lbox_info_dynamic_meta[] = {
>  	{"memory", lbox_info_memory},
>  	{"gc", lbox_info_gc},
>  	{"vinyl", lbox_info_vinyl},
> +	{"listen", lbox_info_listen},
>  	{NULL, NULL}
>  };
>  
> diff --git a/src/lib/core/evio.c b/src/lib/core/evio.c
> index 2152c15e6..fc8f00e0e 100644
> --- a/src/lib/core/evio.c
> +++ b/src/lib/core/evio.c
> @@ -269,6 +269,13 @@ evio_service_bind_addr(struct evio_service *service)
>  		}
>  	}
>  
> +	/*
> +	 * After binding a result address may be different. For
> +	 * example, if a port was 0.
> +	 */
> +	if (sio_getsockname(fd, &service->addr, &service->addr_len) != 0)
> +		goto error;
> +
>  	say_info("%s: bound to %s", evio_service_name(service),
>  		 sio_strfaddr(&service->addr, service->addr_len));
>  
> @@ -400,6 +407,7 @@ evio_service_stop(struct evio_service *service)
>  
>  	if (ev_is_active(&service->ev)) {
>  		ev_io_stop(service->loop, &service->ev);
> +		service->addr_len = 0;
>  	}
>  
>  	if (service->ev.fd >= 0) {
> diff --git a/test/box/info.result b/test/box/info.result
> index af81f7add..4dc888616 100644
> --- a/test/box/info.result
> +++ b/test/box/info.result
> @@ -77,6 +77,7 @@ t
>  - - cluster
>    - gc
>    - id
> +  - listen
>    - lsn
>    - memory
>    - package
> -- 
> 2.21.0 (Apple Git-122.2)
>