From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0E2672BB1E for ; Thu, 4 Oct 2018 07:54:36 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0-1PZreGXoep for ; Thu, 4 Oct 2018 07:54:35 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8A34E2B9F4 for ; Thu, 4 Oct 2018 07:54:35 -0400 (EDT) From: Georgy Kirichenko Subject: [tarantool-patches] Re: [PATCH v2] iproto: introduce a proxy module. Date: Thu, 04 Oct 2018 14:54:27 +0300 Message-ID: <2337967.721MrxTllO@home.lan> In-Reply-To: <20181002180554.1142-1-sergepetrenko@tarantool.org> References: <20181002180554.1142-1-sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart7064719.bJqfncOrQ7"; micalg="pgp-sha256"; protocol="application/pgp-signature" Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Serge Petrenko --nextPart7064719.bJqfncOrQ7 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 --nextPart7064719.bJqfncOrQ7 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEBJFDbU76LsBbgHBsvKOmCX79zb4FAlu1/3MACgkQvKOmCX79 zb7pBQf+K239NyaiE9PIXCF5mnkqHmaSQsv4hAzVgLMqDTHYNh1/m4Gkfrv6o9t1 I0CIvGrQKH4skPcY6aRFT02tQxH7sDcgAP/a5ftTUre6azdj/hAZrNuXndK9wQzu 0KXaniDLY5hUoRgkMIYEWcS8sz/s/hvLtH7fDbpp7zPWj17SME/lsTy8L2SaZS/R 8E1Pc7a9c1vpowCksDzl+kJhF1jcqJuJ1BQr2dQENKBm2VF5oveWa9QNv+7zbKZV b2iKJdNLV/qKJgaZ1IK1qj7dqTo65i7DfE+47cxKEJJKugROw6sVmPx6bJ5lywTP z5dkzErzM7nJ1AyU8jzeW6w9/XsfWQ== =0pZe -----END PGP SIGNATURE----- --nextPart7064719.bJqfncOrQ7--