From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 74A493857007 for ; Wed, 1 Jul 2020 20:59:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 74A493857007 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-454-VRyX3oBXMM6r7FcLcn5vtA-1; Wed, 01 Jul 2020 16:59:44 -0400 X-MC-Unique: VRyX3oBXMM6r7FcLcn5vtA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CC2DA804001 for ; Wed, 1 Jul 2020 20:59:43 +0000 (UTC) Received: from greed.delorie.com (ovpn-113-9.phx2.redhat.com [10.3.113.9]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A11D81001B0B; Wed, 1 Jul 2020 20:59:40 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.14.7/8.14.7) with ESMTP id 061Kxe66006828; Wed, 1 Jul 2020 16:59:40 -0400 From: DJ Delorie To: "Carlos O'Donell" Cc: libc-alpha@sourceware.org Subject: Re: [PATCH 1/4] nss: Introduce In-Reply-To: <7b4dc94f-c411-a1c2-af37-19345d53a0d4@redhat.com> (carlos@redhat.com) Date: Wed, 01 Jul 2020 16:59:40 -0400 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-8.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_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 01 Jul 2020 20:59:47 -0000 V2 to follow once I work through all four patch updates. "Carlos O'Donell" writes: >> +/* Suffix after .so of NSS service modules. */ >> +static const char *const __nss_shlib_revision = LIBNSS_FILES_SO + 15; > > This is magic. > > Suggest: > > /* Start with a known built-in provided service e.g. LIBNSS_FILES_SO > and extract the suffix after the of the NSS service modules e.g. > "libnss_files.so[.2]" => ".2". > We have no API to extract this except through the auto-generated > lib-names.h and some static pointer manipulation e.g. "+ 15". */ Went with: /* Suffix after .so of NSS service modules. This is a bit of magic, but we assume LIBNSS_FILES_SO looks like "libnss_files.so.2" and we want a pointer to the ".2" part. We have no API to extract this except through the auto-generated lib-names.h and some static pointer manipulation. The "-1" accounts for the trailing NUL included in the sizeof. */ static const char *const __nss_shlib_revision = LIBNSS_FILES_SO + sizeof("libnss_files.so") - 1; >> + >> +/* A single-linked list used to implement a mapping from names to NSS > > s/names/service names/g Done. >> + modules. (Most systems only use five or so service modules, so a > > s/service modules/modules/g Done. > Suggest: > > /* Allocate the service NAME with length NAME_LENGTH. If the service > is already allocated in the nss_module_list cache then we return > a pointer to the struct nss_module, otherwise we try to allocate > a new struct nss_module entry and add it to the global nss_modules_list > cache. If we fail to allocate the entry we return NULL. Failure to > allocate the entry is always transient. */ Changed. >> + >> +/* Long enough to store the name of any function. */ > > Suggest: > > /* Long enough to store the name of any function. Currently the > longest function is "getprotobynumber_r" which is 19 including > the NULL termiantor. */ > >> +typedef char function_name[19]; > > Suggest: > > Should we change 19 to `sizeof("getprotobynumber_r")` to be statically > determined by the compiler? This way it's clear what we're trying to do. > It also avoids needing to count characters, you just updated by copy and > pasting the new longest word. Went with: /* Long enough to store the name of any function in the nss_function_name_array list below, as getprotobynumber_r is the longest entry in that list. */ typedef char function_name[sizeof("getprotobynumber_r")]; > Suggest: > > /* Internal implementation of __nss_module_load. */ Changed. > Suggest: > > /* Failing to load the module can be caused by several different scenarios. > One such scenario is that the module has been removed from the disk. > In which case the in-memory version is all that we have, and if the > module->state indidates it is loaded then we can use it. */ Changed. > Suggest: > /* Copy the function pointers locally. */ Went with: /* Look up and store locally all the function pointers we may need later. Doing this now means the data will not change in the future. */ >> + /* Intall the function pointers, following the double-checked > > s/Intall/Install/g Changed. > > Suggest: > > /* Synchronizes with unlocked __nss_module_load atomic_load_acquire. */ Changed. > Suggest: > > /* Turns out the module was already loaded, so close our own handle. > Closing our own handle does not actually unload the modules, only > the reference counter is decremented for the loaded module. */ Went with: /* If the module was already loaded, close our own handle. This does not actually unload the modules, only the reference counter is decremented for the loaded module. */ >> +/* Ensures that MODULE is in a loaded or failed state. */ > > Suggest: > > /* Force the module identified by MODULE to be loaded. We return false > if the module could not be loaded, true otherwise. Loading the module > requires looking up all the possible interface APIs and caching the > results. */ Changed. > Suggest: > > /* Load module MODULE and return a pointer to the module's implementation > of NAME, otherwise return NULL on failure or error. */ Went with: /* Load module MODULE (if it isn't already) and return a pointer to the module's implementation of NAME, otherwise return NULL on failure or error. */