From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Tue, 20 Mar 2018 21:29:20 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH 4/5] session: introduce session_owner
Message-ID: <20180320182920.2pzb2axxxnojd6gc@esperanza>
References: <cover.1521466428.git.v.shpilevoy@tarantool.org>
 <3009c98123cc3be6c5d0b0a0db064757b71faf16.1521466428.git.v.shpilevoy@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3009c98123cc3be6c5d0b0a0db064757b71faf16.1521466428.git.v.shpilevoy@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Mon, Mar 19, 2018 at 04:34:51PM +0300, Vladislav Shpilevoy wrote:
> Session owner stores a session type specific data. For example,
> IProto session has authentication salt, console session has
> file descriptor.
> 
> For #2677 session owner of IProto and console will have push()
> virtual function to do box.session.push, which implementation
> depends on a session type.
> 
> Needed for #2677

> diff --git a/src/box/session.h b/src/box/session.h
> @@ -60,6 +58,49 @@ enum session_type {
>  
>  extern const char *session_type_strs[];
>  
> +struct session_owner_vtab;
> +
> +/**
> + * Object to store session type specific data. For example, IProto
> + * stores iproto_connection, console stores file descriptor.
> + */
> +struct session_owner {

I don't really like the name. Please come up with some alternatives
(session_context, creator, data? dunno) so that we can pick the best
one.

> +	/** Session type. */
> +	enum session_type type;
> +	/** Virtual session owner methods. */
> +	const struct session_owner_vtab *vtab;
> +};
> +
> +struct session_owner_vtab {
> +	/** Allocate a duplicate of an owner. */
> +	struct session_owner *(*dup)(struct session_owner *);
> +	/** Destroy an owner, and free its memory. */
> +	void (*free)(struct session_owner *);
> +	/** Get the descriptor of an owner, if has. Else -1. */
> +	int (*fd)(const struct session_owner *);
> +};
> +
> +static inline struct session_owner *
> +session_owner_dup(struct session_owner *owner)
> +{
> +	return owner->vtab->dup(owner);
> +}
> +
> +static inline void
> +session_owner_delete(struct session_owner *owner)
> +{
> +	owner->vtab->free(owner);
> +}

> diff --git a/src/box/session.cc b/src/box/session.cc
> @@ -112,13 +155,25 @@ session_create(int fd, enum session_type type)
>  	return session;
>  }
>  
> +int
> +session_set_owner(struct session *session, struct session_owner *new_owner)
> +{
> +	struct session_owner *dup = session_owner_dup(new_owner);
> +	if (dup == NULL)
> +		return -1;
> +	if (session->owner != NULL)
> +		session_owner_delete(session->owner);
> +	session->owner = dup;
> +	return 0;
> +}

> diff --git a/src/box/applier.cc b/src/box/applier.cc
> @@ -533,7 +533,9 @@ applier_f(va_list ap)
>  	 * Set correct session type for use in on_replace()
>  	 * triggers.
>  	 */
> -	current_session()->type = SESSION_TYPE_APPLIER;
> +	struct session_owner applier_owner;
> +	session_owner_create(&applier_owner, SESSION_TYPE_APPLIER);
> +	session_set_owner(current_session(), &applier_owner);

I don't like the way you set the owner: first you initialize it on
stack, then duplicate it on heap. This doesn't look good.

Let's try to embed the session_owner structure (or whatever it will be
called) in struct session with some padding so that we don't need to
allocate anything, i.e.:

struct session_owner {
	int type;
	struct session_owner_vtab vtab;
	char pad[128];
};

struct session {
	struct session_owner owner;

We can use static assertions to make sure all deriving structures fit in
(see struct port for example).

Then we would add

iproto_init_session(session, ...)
console_init_session(session, ...)
session_clear_owner(session)

(elaborate the names pls)

that would initialize session->owner appropriately.