Re: [tarantool-patches] [PATCH v2 06/10] session: introduce session vtab and meta

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/04/20 16:25]:
> +int
> +iproto_session_fd(struct session *session)
> +{
> +	struct iproto_connection *conn =
> +		(struct iproto_connection *) session->meta.conn;
> +	return conn->output.fd;
> +}

Nitpick: I would not abbreviate connection when in a member, and
use 'con' as in other places for name of the variable declared on
the stack.

> +/**
> + * Push a message using a protocol, depending on a session type.
> + * @param data Data to push, first argument on a stack.
> + * @retval true Success.
> + * @retval nil, error Error occured.
> + */
> +static int
> +lbox_session_push(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1)
> +		return luaL_error(L, "Usage: box.session.push(data)");
> +
> +	if (session_push(current_session(), NULL) != 0) {
> +		lua_pushnil(L);
> +		luaT_pusherror(L, box_error_last());
> +		return 2;
> +	} else {
> +		lua_pushboolean(L, true);
> +		return 1;
> +	}
> +}

I'm not sure we the calling convention should differ from the rest
of box API, i.e. we should return nil, error rather than
exceptions. Could you run a poll in the community chat?

> +
>  /**
>   * Sets trigger on_access_denied.
>   * For test purposes only.
> @@ -429,6 +451,7 @@ box_lua_session_init(struct lua_State *L)
>  		{"on_disconnect", lbox_session_on_disconnect},
>  		{"on_auth", lbox_session_on_auth},
>  		{"on_access_denied", lbox_session_on_access_denied},
> +		{"push", lbox_session_push},
>  		{NULL, NULL}
>  	};
>  	luaL_register_module(L, sessionlib_name, sessionlib);
> diff --git a/src/box/session.cc b/src/box/session.cc
> index 3d787bd51..4a1397c24 100644
> --- a/src/box/session.cc
> +++ b/src/box/session.cc
> @@ -45,6 +45,20 @@ const char *session_type_strs[] = {
>  	"unknown",
>  };
>  
> +static struct session_vtab generic_session_vtab = {
> +	/* .push = */ generic_session_push,
> +	/* .fd = */ generic_session_fd,
> +	/* .sync = */ generic_session_sync,
> +};
> +
> +struct session_vtab session_vtab_registry[] = {
> +	/* BACKGROUND */ generic_session_vtab,
> +	/* BINARY */ generic_session_vtab,
> +	/* CONSOLE */ generic_session_vtab,
> +	/* REPL */ generic_session_vtab,
> +	/* APPLIER */ generic_session_vtab,
> +};
> +
>  static struct mh_i64ptr_t *session_registry;
>  
>  struct mempool session_pool;
> @@ -79,7 +93,7 @@ session_on_stop(struct trigger *trigger, void * /* event */)
>  }
>  
>  struct session *
> -session_create(int fd, enum session_type type)
> +session_create(enum session_type type)
>  {
>  	struct session *session =
>  		(struct session *) mempool_alloc(&session_pool);
> @@ -89,8 +103,7 @@ session_create(int fd, enum session_type type)
>  		return NULL;
>  	}
>  	session->id = sid_max();
> -	session->fd =  fd;
> -	session->sync = 0;
> +	memset(&session->meta, 0, sizeof(session->meta));
>  	session->type = type;
>  	/* For on_connect triggers. */
>  	credentials_init(&session->credentials, guest_user->auth_token,
> @@ -110,12 +123,12 @@ session_create(int fd, enum session_type type)
>  }
>  
>  struct session *
> -session_create_on_demand(int fd)
> +session_create_on_demand()
>  {
>  	assert(fiber_get_session(fiber()) == NULL);
>  
>  	/* Create session on demand */
> -	struct session *s = session_create(fd, SESSION_TYPE_BACKGROUND);
> +	struct session *s = session_create(SESSION_TYPE_BACKGROUND);
>  	if (s == NULL)
>  		return NULL;
>  	s->fiber_on_stop = {
> @@ -278,3 +291,27 @@ access_check_universe(user_access_t access)
>  	}
>  	return 0;
>  }
> +
> +int
> +generic_session_push(struct session *session, struct port *port)
> +{
> +	(void) port;
> +	const char *name =
> +		tt_sprintf("Session '%s'", session_type_strs[session->type]);
> +	diag_set(ClientError, ER_UNSUPPORTED, name, "push()");
> +	return -1;
> +}
> +
> +int
> +generic_session_fd(struct session *session)
> +{
> +	(void) session;
> +	return -1;
> +}
> +
> +int64_t
> +generic_session_sync(struct session *session)
> +{
> +	(void) session;
> +	return 0;
> +}
> diff --git a/src/box/session.h b/src/box/session.h
> index c387e6f95..e583c8c6b 100644
> --- a/src/box/session.h
> +++ b/src/box/session.h
> @@ -41,6 +41,9 @@
>  extern "C" {
>  #endif /* defined(__cplusplus) */
>  
> +struct port;
> +struct session_vtab;
> +
>  void
>  session_init();
>  
> @@ -58,6 +61,23 @@ enum session_type {
>  
>  extern const char *session_type_strs[];
>  
> +/**
> + * Session meta is used in different ways by sessions of different
> + * types, and allows to do not store attributes in struct session,
> + * that are used only by a session of particular type.
> + */
> +struct session_meta {
> +	union {
> +		/** IProto connection meta. */
> +		struct {
> +			uint64_t sync;
> +			void *conn;
> +		};
> +		/** Only by console is used. */
> +		int fd;
> +	};
> +};
> +
>  /**

Otherwise the patch LGTM.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov