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

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

[Vladislav Shpilevoy]

Box.cfg{listen = 0} automatically chooses a port. But it was
impossible to learn a real port the instance is bound to.

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.
+ */
+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.
+ */
+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)

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

[Vladislav Shpilevoy]

Sorry, forgot a test. Will add tomorrow.

On 12/11/2019 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.
> 
> 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
> 

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

[Vladislav Shpilevoy]

Added a test. Now, please, do a review. Especially
Alexander - AFAIU you need this feature.

====================================================================

diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index d529447bb..569b5f463 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(104)
+test:plan(108)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -592,6 +592,18 @@ box.cfg{read_only=true}
 ]]
 test:is(run_script(code), PANIC, "panic on bootstrapping a read-only instance as master")
 
+--
+-- gh-4620: box.info.listen.
+--
+box.cfg{listen = box.NULL}
+test:is(nil, box.info.listen, 'no cfg.listen - no info.listen')
+
+box.cfg{listen = '127.0.0.1:0'}
+test:ok(box.info.listen:match('127.0.0.1'), 'real IP in info.listen')
+test:ok(not box.info.listen:match(':0'), 'real port in info.listen')
+
+box.cfg{listen = box.NULL}
+test:is(nil, box.info.listen, 'cfg.listen reset drops info.listen')
 
 test:check()
 os.exit(0)

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

[Sergey Ostanevich]

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)
> 

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

[Sergey Ostanevich]

Vlad, after thinking over the patch some more I've got a feeling it can
break backward compatibility. Can Tarantool with these changes in iproto
successfully communicate to the one without the changes?

@Totktonada we need to formulate the cross-version testing. 

Regards,
Sergos


On 21 фев 22:51, Sergey Ostanevich wrote:
> 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)
> > 

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

[Vladislav Shpilevoy]

On 22/02/2020 09:34, Sergey Ostanevich wrote:
> Vlad, after thinking over the patch some more I've got a feeling it can
> break backward compatibility. Can Tarantool with these changes in iproto
> successfully communicate to the one without the changes?

Why would it break anything? This has nothing to do with the protocol
or network communication. This is just a new record in box.info table.
I don't see how is it related to cross version interaction.

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

[Vladislav Shpilevoy]

Since the patch is very old, there was no ChangeLog
record added. Here it is:

@ChangeLog
- box.info.listen - new record in box.info, which shows a
  real port when bound to port 0. For example, if box.cfg
  'listen' parameter was set to '127.0.0.1:0', box.info.listen
  will show '127.0.0.1:<real port > 0>' (gh-4620).

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

[Vladislav Shpilevoy]

Hi! Thanks for the review!

>> Box.cfg{listen = 0} automatically chooses a port. But it was
>> impossible to learn a real port the instance is bound to.
>                 ^^^^^ obtain?

Done.

>> 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?

Done.

>> + */
>> +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);
>> +}
>> 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

Done.

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

[Sergey Ostanevich]

Thanks, LGTM. 
The misunderstanding with protocol was my off-the-source fault.

Sergos


On 22 фев 16:00, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
> 
> >> Box.cfg{listen = 0} automatically chooses a port. But it was
> >> impossible to learn a real port the instance is bound to.
> >                 ^^^^^ obtain?
> 
> Done.
> 
> >> 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?
> 
> Done.
> 
> >> + */
> >> +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);
> >> +}
> >> 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
> 
> Done.

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

[Serge Petrenko]

[-- Attachment #1: Type: text/plain, Size: 6405 bytes --]

Hi! Thanks for the patch!
Alexander asked me to do the 2nd review.

The patch LGTM.

Sorry for nitpicking,  but looks like your changelog request has a typo in it:
> @ChangeLog
> - box.info.listen - new record in box.info <http://box.info/>, which shows a
>  real port when bound to port 0. For example, if box.cfg
>  'listen' parameter was set to '127.0.0.1:0', box.info.listen
>  will show '127.0.0.1:<real port > 0>' (gh-4620).
just <real port>, without 0>, right?

--
Serge Petrenko
sergepetrenko@tarantool.org




> 13 нояб. 2019 г., в 01:03, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Box.cfg{listen = 0} automatically chooses a port. But it was
> impossible to learn a real port the instance is bound to.
> 
> 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.
> + */
> +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.
> + */
> +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)
> 


[-- Attachment #2: Type: text/html, Size: 16261 bytes --]

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

[Vladislav Shpilevoy]

Thanks for the review!

On 18/03/2020 19:18, Serge Petrenko wrote:
> Hi! Thanks for the patch!
> Alexander asked me to do the 2nd review.
> 
> The patch LGTM.
> 
> Sorry for nitpicking,  but looks like your changelog request has a typo in it:
>> @ChangeLog
>> - box.info.listen - new record in box.info <http://box.info>, which shows a
>>  real port when bound to port 0. For example, if box.cfg
>>  'listen' parameter was set to '127.0.0.1:0', box.info.listen
>>  will show '127.0.0.1:<real port > 0>' (gh-4620).
> just <real port>, without 0>, right?

Nope, I meant > 0. I wanted to emphasize, that the port is never
shown as 0 anymore. It is always some real port > 0.

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

[Serge Petrenko]

> 19 марта 2020 г., в 01:38, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Thanks for the review!
> 
> On 18/03/2020 19:18, Serge Petrenko wrote:
>> Hi! Thanks for the patch!
>> Alexander asked me to do the 2nd review.
>> 
>> The patch LGTM.
>> 
>> Sorry for nitpicking,  but looks like your changelog request has a typo in it:
>>> @ChangeLog
>>> - box.info.listen - new record in box.info <http://box.info>, which shows a
>>>  real port when bound to port 0. For example, if box.cfg
>>>  'listen' parameter was set to '127.0.0.1:0', box.info.listen
>>>  will show '127.0.0.1:<real port > 0>' (gh-4620).
>> just <real port>, without 0>, right?
> 
> Nope, I meant > 0. I wanted to emphasize, that the port is never
> shown as 0 anymore. It is always some real port > 0.

Ah, I see. Sorry for the misunderstanding.

--
Serge Petrenko
sergepetrenko@tarantool.org

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

[Kirill Yukhin]

Hello,

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.
> 
> 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

I've checked your patch into master.

--
Regards, Kirill Yukhin