From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 3051F385DC31 for ; Thu, 17 Mar 2022 00:48:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3051F385DC31 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-5-s3shWDVFNdiVaHXuAiKoTA-1; Wed, 16 Mar 2022 20:48:21 -0400 X-MC-Unique: s3shWDVFNdiVaHXuAiKoTA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 13901101AA57; Thu, 17 Mar 2022 00:48:21 +0000 (UTC) Received: from greed.delorie.com (ovpn-112-4.rdu2.redhat.com [10.10.112.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F34C3215688A; Thu, 17 Mar 2022 00:48:20 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.15.2/8.15.2) with ESMTP id 22H0mJAx1742411; Wed, 16 Mar 2022 20:48:20 -0400 From: DJ Delorie To: Siddhesh Poyarekar Cc: libc-alpha@sourceware.org Subject: Re: [PATCH v2 04/12] gaih_inet: Simplify service resolution In-Reply-To: <20220314094835.1159523-5-siddhesh@sourceware.org> (message from Siddhesh Poyarekar via Libc-alpha on Mon, 14 Mar 2022 15:18:27 +0530) Date: Wed, 16 Mar 2022 20:48:19 -0400 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Mar 2022 00:48:26 -0000 Siddhesh Poyarekar via Libc-alpha writes: > Refactor the code to split out the service resolution code into a > separate function. Allocate the service tuples array just once to the > size of the typeproto array, thus avoiding the unnecessary pointer > chasing and stack allocations. LGTM Reviewed-by: DJ Delorie > struct gaih_servtuple > { > - struct gaih_servtuple *next; > int socktype; > int protocol; > int port; > + bool set; > }; > > -static const struct gaih_servtuple nullserv; > - Ok. > @@ -180,11 +178,11 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp, > } > while (r); > > - st->next = NULL; > st->socktype = tp->socktype; > st->protocol = ((tp->protoflag & GAI_PROTO_PROTOANY) > ? req->ai_protocol : tp->protocol); > st->port = s->s_port; > + st->set = true; Ok. > @@ -375,20 +373,11 @@ process_canonname (const struct addrinfo *req, const char *orig_name, > } > > static int > -gaih_inet (const char *name, const struct gaih_service *service, > - const struct addrinfo *req, struct addrinfo **pai, > - unsigned int *naddrs, struct scratch_buffer *tmpbuf) > +get_servtuples (const struct gaih_service *service, const struct addrinfo *req, > + struct gaih_servtuple *st, struct scratch_buffer *tmpbuf) > { Split out but the names of passed variables remain the same; ok. > + int i; > const struct gaih_typeproto *tp = gaih_inet_typeproto; > - struct gaih_servtuple *st = (struct gaih_servtuple *) &nullserv; > - struct gaih_addrtuple *at = NULL; > - bool got_ipv6 = false; > - char *canon = NULL; > - const char *orig_name = name; > - > - /* Reserve stack memory for the scratch buffer in the getaddrinfo > - function. */ > - size_t alloca_used = sizeof (struct scratch_buffer); This part done in the parent function; ok. > @@ -410,98 +399,88 @@ gaih_inet (const char *name, const struct gaih_service *service, > } > } > > - int port = 0; > - if (service != NULL) > + if (service != NULL && (tp->protoflag & GAI_PROTO_NOSERVICE) != 0) > + return -EAI_SERVICE; > + > + if (service == NULL || service->num >= 0) > { > - if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0) > - return -EAI_SERVICE; > + int port = service != NULL ? htons (service->num) : 0; Ok. > + if (req->ai_socktype || req->ai_protocol) > { > + st[0].socktype = tp->socktype; > + st[0].protocol = ((tp->protoflag & GAI_PROTO_PROTOANY) > + ? req->ai_protocol : tp->protocol); > + st[0].port = port; > + st[0].set = true; > + return 0; > + } ok. > + /* Neither socket type nor protocol is set. Return all socket types > + we know about. */ > + for (i = 0, ++tp; tp->name[0]; ++tp) > + if (tp->defaultflag) > + { > + st[i].socktype = tp->socktype; > + st[i].protocol = tp->protocol; > + st[i].port = port; > + st[i++].set = true; > + } Ok. > - if (service->num < 0) > - if (tp->name[0]) > - { > - st = (struct gaih_servtuple *) > - alloca_account (sizeof (struct gaih_servtuple), alloca_used); > - > - int rc = gaih_inet_serv (service->name, tp, req, st, tmpbuf); > - if (__glibc_unlikely (rc != 0)) > - return rc; > - } > - else > - { > - struct gaih_servtuple **pst = &st; > - for (tp++; tp->name[0]; tp++) > - { > - struct gaih_servtuple *newp; > > - if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0) > - continue; > > - if (req->ai_socktype != 0 > - && req->ai_socktype != tp->socktype) > - continue; > - if (req->ai_protocol != 0 > - && !(tp->protoflag & GAI_PROTO_PROTOANY) > - && req->ai_protocol != tp->protocol) > - continue; > > - newp = (struct gaih_servtuple *) > - alloca_account (sizeof (struct gaih_servtuple), > - alloca_used); > + return 0; > + } Ok. > - if (gaih_inet_serv (service->name, > - tp, req, newp, tmpbuf) != 0) > - continue; Ok. > + if (tp->name[0]) > + return gaih_inet_serv (service->name, tp, req, st, tmpbuf); Ok. > + for (i = 0, tp++; tp->name[0]; tp++) > + if ((tp->protoflag & GAI_PROTO_NOSERVICE) != 0) > + continue; > + if (req->ai_socktype != 0 > + && req->ai_socktype != tp->socktype) > + continue; > + if (req->ai_protocol != 0 > + && !(tp->protoflag & GAI_PROTO_PROTOANY) > + && req->ai_protocol != tp->protocol) > + continue; > + if (gaih_inet_serv (service->name, > + tp, req, &st[i], tmpbuf) != 0) > + continue; > + i++; > + if (!st[0].set) > + return -EAI_SERVICE; > + > + return 0; > +} Ok. > - *pst = newp; > - pst = &(newp->next); > - } > - if (st == (struct gaih_servtuple *) &nullserv) > - return -EAI_SERVICE; > - } > - } > - else > - { > - port = htons (service->num); > - goto got_port; > - } > - } > - else > { > - got_port: > > - if (req->ai_socktype || req->ai_protocol) > - { > - st = alloca_account (sizeof (struct gaih_servtuple), alloca_used); > - st->next = NULL; > - st->socktype = tp->socktype; > - st->protocol = ((tp->protoflag & GAI_PROTO_PROTOANY) > - ? req->ai_protocol : tp->protocol); > - st->port = port; > - } > - else > - { > - /* Neither socket type nor protocol is set. Return all socket types > - we know about. */ > - struct gaih_servtuple **lastp = &st; > - for (++tp; tp->name[0]; ++tp) > - if (tp->defaultflag) > - { > - struct gaih_servtuple *newp; > > - newp = alloca_account (sizeof (struct gaih_servtuple), > - alloca_used); > - newp->next = NULL; > - newp->socktype = tp->socktype; > - newp->protocol = tp->protocol; > - newp->port = port; > > - *lastp = newp; > - lastp = &newp->next; > - } > - } > } Ok. > + > +static int > +gaih_inet (const char *name, const struct gaih_service *service, > + const struct addrinfo *req, struct addrinfo **pai, > + unsigned int *naddrs, struct scratch_buffer *tmpbuf) > +{ > + struct gaih_servtuple st[sizeof (gaih_inet_typeproto) > + / sizeof (struct gaih_typeproto)] = {0}; > + > + struct gaih_addrtuple *at = NULL; > + bool got_ipv6 = false; > + char *canon = NULL; > + const char *orig_name = name; > + > + /* Reserve stack memory for the scratch buffer in the getaddrinfo > + function. */ > + size_t alloca_used = sizeof (struct scratch_buffer); > + > + int rc; > + if ((rc = get_servtuples (service, req, st, tmpbuf)) != 0) > + return rc; > + Ok. > - struct gaih_servtuple *st2; Ok. > - for (st2 = st; st2 != NULL; st2 = st2->next) > + for (int i = 0; st[i].set; i++) Ok. > - ai->ai_socktype = st2->socktype; > - ai->ai_protocol = st2->protocol; > + ai->ai_socktype = st[i].socktype; > + ai->ai_protocol = st[i].protocol; Ok. > - sin6p->sin6_port = st2->port; > + sin6p->sin6_port = st[i].port; Ok. > - sinp->sin_port = st2->port; > + sinp->sin_port = st[i].port; Ok.