[tarantool-patches] Re: [PATCH v2] iproto: introduce a proxy module.

[Georgy Kirichenko <georgy@tarantool.org>]

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

Hi and thank you for the patch! Overall it looks good but I have some 
comments.
 - I would like if you created a dedicated fiber pool and cbus endpoint to
   perform IPROTO<->TX interconnection. In that case iproto and tx
   initialization could be more clearly splitted.
 - I think we could have only one hash table to locate a corresponding
   connection with "username@uri" instead of two-level routing.
 - It should be fine to introduce some error injections.
 - If you allocate and free a lots of same space objects then mempool
   is better option than (m/c)alloc/free

Also I have some comments inline.
> +
> +	instance->is_ro = is_ro;
> +	instance->is_local = is_local;
> +	if (is_local) {
> +		/* There can be only one local instance. */
> +		assert(local_instance == NULL);
> +		assert(box_is_configured());
This function works in the IPROTO thread and calling TX threaded function may 
lead to undefined behavior.
> +		local_instance = instance;
> +	} else if (uri_parse(&instance->uri, uri) || !instance->uri.service) {
> +			free(instance);
> +			diag_set(ClientError, ER_CFG, "uri",
> +				 "expected host:service or /unix.socket");
> +			return NULL;
> +	}
> +
> +	rlist_add_tail(&proxy_instances, &instance->link);
> +

> +
> +		if (!is_proxy_configured) {
> +			/*
> +			 * This can't throw, but should not be
> +			 * done in case of exception.
> +			 */
> +process_locally:
> +			cpipe_push_input(&tx_pipe, &msg->base);
> +			continue;
> +		}
> +
> +		struct proxy_instance *instance = proxy_find_instance(msg);
> +		if (instance == NULL) {
I think some actions with request should be done, at least clearing an input 
buffer space.
> +			diag_log();
> +			continue;
> +		}
> +		if (instance == local_instance) {
> +			if (msg->header.type != IPROTO_AUTH)
> +				goto process_locally;
> +			struct proxy_auth_msg *m = (struct proxy_auth_msg 

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]