diff mbox

[RFC,3/4,BZ,17083] NSS: Introduce gethostbyname5_r function

Message ID 20170905160530.19525-4-stlman@poczta.fm
State New, archived
Headers

Commit Message

Łukasz Stelmach Sept. 5, 2017, 4:05 p.m. UTC 
  Add af (address family) to the list of arguments of gethostbyname4_r
function in files, dns, nis and nisplus module and rename the function to
gethostbyname5_r. Pass the af value to the underlying parsing functions.
Define gethostbyname4_r as a wrapper for the former passing af set
to AF_UNSPEC.

Signed-off-by: Łukasz Stelmach <stlman@poczta.fm>
---
 nis/Versions                    |  3 ++-
 nis/nss_nis/nis-hosts.c         | 13 +++++++++++--
 nis/nss_nisplus/nisplus-hosts.c | 13 +++++++++++--
 nss/Versions                    |  1 +
 nss/nss_files/files-hosts.c     | 13 +++++++++++--
 resolv/Versions                 |  2 +-
 resolv/nss_dns/dns-host.c       | 32 ++++++++++++++++++++++++++++++--
 7 files changed, 67 insertions(+), 10 deletions(-)

Comments

Florian Weimer Sept. 12, 2017, 10:03 a.m. UTC | #1 
On 09/05/2017 06:05 PM, Łukasz Stelmach wrote:
> Add af (address family) to the list of arguments of gethostbyname4_r
> function in files, dns, nis and nisplus module and rename the function to
> gethostbyname5_r. Pass the af value to the underlying parsing functions.
> Define gethostbyname4_r as a wrapper for the former passing af set
> to AF_UNSPEC.

I'd prefer if we have a single ultimate name resolution function,
instead of doing function 5 now, and function 6 later, when we have
dealt with the malloc matters.

One way to deal with the dlmopen/static dlopen issue would be to define
a deallocation function which goes with the 5 function.  If this
function called free, it would automatically end up at the right definition.

Thanks,
Florian
Łukasz Stelmach Sept. 12, 2017, 10:44 a.m. UTC | #2 
Dnia 12 września 2017 12:03:00 CEST, Florian Weimer <fweimer@redhat.com> napisał(a):
>On 09/05/2017 06:05 PM, Łukasz Stelmach wrote:
>> Add af (address family) to the list of arguments of gethostbyname4_r
>> function in files, dns, nis and nisplus module and rename the
>function to
>> gethostbyname5_r. Pass the af value to the underlying parsing
>functions.
>> Define gethostbyname4_r as a wrapper for the former passing af set
>> to AF_UNSPEC.
>
>I'd prefer if we have a single ultimate name resolution function,
>instead of doing function 5 now, and function 6 later, when we have
>dealt with the malloc matters.

This sounds like a major debate. Do you know who's in charge in this parts of glibc?

>One way to deal with the dlmopen/static dlopen issue would be to define
>a deallocation function which goes with the 5 function.  If this
>function called free, it would automatically end up at the right
>definition.

This means almost nothing to me (-; which means, probably I am not the right guy to do it.
Florian Weimer Sept. 12, 2017, 11 a.m. UTC | #3 
On 09/12/2017 12:44 PM, Łukasz Stelmach wrote:
> Dnia 12 września 2017 12:03:00 CEST, Florian Weimer <fweimer@redhat.com> napisał(a):
>> On 09/05/2017 06:05 PM, Łukasz Stelmach wrote:
>>> Add af (address family) to the list of arguments of gethostbyname4_r
>>> function in files, dns, nis and nisplus module and rename the
>> function to
>>> gethostbyname5_r. Pass the af value to the underlying parsing
>> functions.
>>> Define gethostbyname4_r as a wrapper for the former passing af set
>>> to AF_UNSPEC.
>>
>> I'd prefer if we have a single ultimate name resolution function,
>> instead of doing function 5 now, and function 6 later, when we have
>> dealt with the malloc matters.
> 
> This sounds like a major debate. Do you know who's in charge in this parts of glibc?

It's a community project.  We don't have strong component ownership.  I
have taken care of networking-related matters for some time now.

>> One way to deal with the dlmopen/static dlopen issue would be to define
>> a deallocation function which goes with the 5 function.  If this
>> function called free, it would automatically end up at the right
>> definition.
> 
> This means almost nothing to me (-; which means, probably I am not the right guy to do it. 

We currently pass a fixed-size buffer into the NSS service module.  The
NSS service module can fail with
NSS_STATUS_TRYAGAIN/NETDB_INTERNAL/ERANGE to indicate that the buffer is
too small.  The caller is supposed to try again with a larger buffer.

This is very inefficient.  It can result in a large number of DNS
queries if there are many addresses (A/AAAA over UDP, then over TCP).

We could avoid this if the NSS service module allocates space for the
result because it knows the required size.  But the module could use a
different malloc than the main program (like Windows DLLs, but for GNU,
this is a special case which does not apply to regular DSO usage).
There are two ways to fix this: The NSS module could provide a
deallocation function, or we could add a mechanism to the dynamic linker
that ensures the mallocs always match.  The second approach is quite
hard to implement.  The first approach has other benefits (e.g., the
service module could return a pointer into a cache and use reference
counting instead of direct malloc/free to manage the memory).

I don't see your name in the maintainers list on the wiki.  Since your
current patches already are substantial contribution, we need to discuss
the matter of copyright assignment:

https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment

I suggest you contact the FSF to eliminate that roadblock before working
on these patches further.

Thanks,
Florian
Łukasz Stelmach Sept. 12, 2017, 12:46 p.m. UTC | #4 
Dnia 12 września 2017 13:00:41 CEST, Florian Weimer <fweimer@redhat.com> napisał(a):

>I don't see your name in the maintainers list on the wiki.  Since your
>current patches already are substantial contribution, we need to
>discuss
>the matter of copyright assignment:
>
>https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment
>
>I suggest you contact the FSF to eliminate that roadblock before
>working
>on these patches further.

I suppose, I should send https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future. Am I right?
Florian Weimer Sept. 12, 2017, 5 p.m. UTC | #5 
On 09/12/2017 02:46 PM, Łukasz Stelmach wrote:
> Dnia 12 września 2017 13:00:41 CEST, Florian Weimer <fweimer@redhat.com> napisał(a):
> 
>> I don't see your name in the maintainers list on the wiki.  Since your
>> current patches already are substantial contribution, we need to
>> discuss
>> the matter of copyright assignment:
>>
>> https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment
>>
>> I suggest you contact the FSF to eliminate that roadblock before
>> working
>> on these patches further.
> 
> I suppose, I should send https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future. Am I right?

Yes, it's a valid option if you want to go that route.

Thanks,
Florian
diff mbox

Patch

diff --git a/nis/Versions b/nis/Versions
index ef9a512417..537f0aa3c0 100644
--- a/nis/Versions
+++ b/nis/Versions
@@ -96,6 +96,7 @@  libnss_nis {
     _nss_nis_setnetgrent; _nss_nis_setprotoent; _nss_nis_setpwent;
     _nss_nis_setrpcent; _nss_nis_setservent; _nss_nis_setspent;
     _nss_nis_initgroups_dyn; _nss_nis_gethostbyname4_r;
+    _nss_nis_gethostbyname5_r;
   }
 }
 
@@ -126,6 +127,6 @@  libnss_nisplus {
     _nss_nisplus_setnetent; _nss_nisplus_setnetgrent; _nss_nisplus_setprotoent;
     _nss_nisplus_setpwent; _nss_nisplus_setrpcent; _nss_nisplus_setservent;
     _nss_nisplus_setspent; _nss_nisplus_initgroups_dyn;
-    _nss_nisplus_gethostbyname4_r;
+    _nss_nisplus_gethostbyname4_r; _nss_nisplus_gethostbyname5_r;
   }
 }
diff --git a/nis/nss_nis/nis-hosts.c b/nis/nss_nis/nis-hosts.c
index f64dbdaecb..3a156aa7ee 100644
--- a/nis/nss_nis/nis-hosts.c
+++ b/nis/nss_nis/nis-hosts.c
@@ -454,7 +454,7 @@  _nss_nis_gethostbyaddr_r (const void *addr, socklen_t addrlen, int af,
 
 
 enum nss_status
-_nss_nis_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+_nss_nis_gethostbyname5_r (const char *name, int af, struct gaih_addrtuple **pat,
 			   char *buffer, size_t buflen, int *errnop,
 			   int *herrnop, int32_t *ttlp)
 {
@@ -530,7 +530,7 @@  _nss_nis_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
   buflen -= pad;
 
   struct hostent host;
-  int parse_res = parse_line (result, &host, data, buflen, errnop, AF_UNSPEC,
+  int parse_res = parse_line (result, &host, data, buflen, errnop, af,
 			      0);
   if (__glibc_unlikely (parse_res < 1))
     {
@@ -565,3 +565,12 @@  _nss_nis_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
 
   return NSS_STATUS_SUCCESS;
 }
+
+enum nss_status
+_nss_nis_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+			   char *buffer, size_t buflen, int *errnop,
+			   int *herrnop, int32_t *ttlp)
+{
+  return _nss_nis_gethostbyname5_r (name, AF_UNSPEC, pat, buffer, buflen,
+				    errnop, herrnop, ttlp);
+}
diff --git a/nis/nss_nisplus/nisplus-hosts.c b/nis/nss_nisplus/nisplus-hosts.c
index 31dfd31fbd..d9c5d7a974 100644
--- a/nis/nss_nisplus/nisplus-hosts.c
+++ b/nis/nss_nisplus/nisplus-hosts.c
@@ -579,13 +579,13 @@  _nss_nisplus_gethostbyaddr_r (const void *addr, socklen_t addrlen, int af,
 
 
 enum nss_status
-_nss_nisplus_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+_nss_nisplus_gethostbyname5_r (const char *name, int af, struct gaih_addrtuple **pat,
 			       char *buffer, size_t buflen, int *errnop,
 			       int *herrnop, int32_t *ttlp)
 {
   struct hostent host;
 
-  enum nss_status status = internal_gethostbyname2_r (name, AF_UNSPEC, &host,
+  enum nss_status status = internal_gethostbyname2_r (name, af, &host,
 						      buffer, buflen,
 						      errnop, herrnop, 0);
   if (__glibc_likely (status == NSS_STATUS_SUCCESS))
@@ -617,3 +617,12 @@  _nss_nisplus_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
 
   return status;
 }
+
+enum nss_status
+_nss_nisplus_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+			       char *buffer, size_t buflen, int *errnop,
+			       int *herrnop, int32_t *ttlp)
+{
+  return _nss_nisplus_gethostbyname5_r (name, AF_UNSPEC, pat, buffer,
+					buflen, errnop, herrnop, ttlp);
+}
diff --git a/nss/Versions b/nss/Versions
index 50268ed9b5..72adf4c806 100644
--- a/nss/Versions
+++ b/nss/Versions
@@ -44,6 +44,7 @@  libnss_files {
     _nss_files_gethostbyname2_r;
     _nss_files_gethostbyname3_r;
     _nss_files_gethostbyname4_r;
+    _nss_files_gethostbyname5_r;
     _nss_files_gethostbyname_r;
     _nss_files_gethostent_r;
 
diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c
index 28eb73fb2d..c6067cc641 100644
--- a/nss/nss_files/files-hosts.c
+++ b/nss/nss_files/files-hosts.c
@@ -404,7 +404,7 @@  _nss_files_gethostbyname2_r (const char *name, int af, struct hostent *result,
 }
 
 enum nss_status
-_nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+_nss_files_gethostbyname5_r (const char *name, int af, struct gaih_addrtuple **pat,
 			     char *buffer, size_t buflen, int *errnop,
 			     int *herrnop, int32_t *ttlp)
 {
@@ -428,7 +428,7 @@  _nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
 
 	  struct hostent result;
 	  status = internal_getent (stream, &result, buffer, buflen, errnop,
-				    herrnop, AF_UNSPEC, 0, &in6_zone_id);
+				    herrnop, af, 0, &in6_zone_id);
 	  if (status != NSS_STATUS_SUCCESS)
 	    break;
 
@@ -518,3 +518,12 @@  _nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
 
   return status;
 }
+
+enum nss_status
+_nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+			     char *buffer, size_t buflen, int *errnop,
+			     int *herrnop, int32_t *ttlp)
+{
+  return _nss_files_gethostbyname5_r (name, AF_UNSPEC, pat, buffer, buflen,
+				      errnop, herrnop, ttlp);
+}
diff --git a/resolv/Versions b/resolv/Versions
index b05778d965..2ae1048908 100644
--- a/resolv/Versions
+++ b/resolv/Versions
@@ -96,7 +96,7 @@  libnss_dns {
     _nss_dns_gethostbyname_r; _nss_dns_getnetbyaddr_r;
     _nss_dns_getnetbyname_r; _nss_dns_getcanonname_r;
     _nss_dns_gethostbyaddr2_r;
-    _nss_dns_gethostbyname4_r;
+    _nss_dns_gethostbyname4_r; _nss_dns_gethostbyname5_r;
   }
 }
 
diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
index 7cd54ab504..dddb8f4bef 100644
--- a/resolv/nss_dns/dns-host.c
+++ b/resolv/nss_dns/dns-host.c
@@ -308,7 +308,7 @@  _nss_dns_gethostbyname_r (const char *name, struct hostent *result,
 
 
 enum nss_status
-_nss_dns_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+_nss_dns_gethostbyname5_r (const char *name, int af, struct gaih_addrtuple **pat,
 			   char *buffer, size_t buflen, int *errnop,
 			   int *herrnop, int32_t *ttlp)
 {
@@ -344,10 +344,28 @@  _nss_dns_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
   int nans2p = 0;
   int resplen2 = 0;
   int ans2p_malloced = 0;
+  int type;
 
   int olderr = errno;
   enum nss_status status;
-  int n = __res_context_search (ctx, name, C_IN, T_QUERY_A_AND_AAAA,
+
+  switch (af) {
+  case AF_UNSPEC:
+    type = T_QUERY_A_AND_AAAA;
+    break;
+  case AF_INET:
+    type = T_A;
+    break;
+  case AF_INET6:
+    type = T_AAAA;
+    break;
+  default:
+    *herrnop = NO_DATA;
+    *errnop = EAFNOSUPPORT;
+    return NSS_STATUS_UNAVAIL;
+  }
+
+  int n = __res_context_search (ctx, name, C_IN, type,
 				host_buffer.buf->buf, 2048, &host_buffer.ptr,
 				&ans2p, &nans2p, &resplen2, &ans2p_malloced);
   if (n >= 0)
@@ -397,6 +415,16 @@  _nss_dns_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
 }
 
 
+enum nss_status
+_nss_dns_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat,
+			   char *buffer, size_t buflen, int *errnop,
+			   int *herrnop, int32_t *ttlp)
+{
+  return _nss_dns_gethostbyname5_r (name, AF_UNSPEC, pat, buffer, buflen,
+				    errnop, herrnop,ttlp);
+}
+
+
 extern enum nss_status _nss_dns_gethostbyaddr2_r (const void *addr,
 						  socklen_t len, int af,
 						  struct hostent *result,