Michal Privoznik wrote:
> On 25.03.2016 08:43, Roman Bogorodskiy wrote:
> > * tools/nss/libvirt_nss.[ch]: add BSD-comptabile wrappers and
> > register via the nss_module_register() interface
> > * m4/virt-nss.m4: add checks if we're building NSS for FreeBSD
> > * tools/Makefile.am: handle target library name differences, as
> > Linux needs libnss_libvirt.so.2 and FreeBSD needs
> > nss_libvirt.so.1. Also, different syms files have to be used
> > as Linux needs to export all the methods while FreeBSD
> > only needs to have nss_module_register()
> > * libvirt_nss_bsd.syms: FreeBSD syms file
> > ---
> > m4/virt-nss.m4 | 12 +++-
> > tools/Makefile.am | 16 ++++-
> > tools/nss/libvirt_nss.c | 140 +++++++++++++++++++++++++++++++++++++++--
> > tools/nss/libvirt_nss.h | 9 +++
> > tools/nss/libvirt_nss_bsd.syms | 9 +++
> > 5 files changed, 179 insertions(+), 7 deletions(-)
> > create mode 100644 tools/nss/libvirt_nss_bsd.syms
> >
> > diff --git a/m4/virt-nss.m4 b/m4/virt-nss.m4
> > index 3fa4ad3..a8ed8b9 100644
> > --- a/m4/virt-nss.m4
> > +++ b/m4/virt-nss.m4
> > @@ -23,6 +23,7 @@ AC_DEFUN([LIBVIRT_CHECK_NSS],[
> > [enable Name Servie Switch plugin for resolving guest IP addresses])],
> > [], [with_nss_plugin=check])
> >
> > + bsd_nss=no
> > fail=0
> > if test "x$with_nss_plugin" != "xno" ; then
> > AC_CHECK_HEADERS([nss.h], [
> > @@ -39,11 +40,20 @@ AC_DEFUN([LIBVIRT_CHECK_NSS],[
> >
> > if test "x$with_nss_plugin" = "xyes" ; then
> > AC_DEFINE_UNQUOTED([NSS], 1, [whether nss plugin is enabled])
> > +
> > + AC_CHECK_TYPES([ns_mtab, nss_module_unregister_fn],
> > + [AC_DEFINE([HAVE_BSD_NSS],
> > + [1],
> > + [whether using BSD style NSS])
> > + bsd_nss=yes
> > + ],
> > + [],
> > + [#include <nsswitch.h>])
> > fi
> > fi
> >
> > AM_CONDITIONAL(WITH_NSS, [test "x$with_nss_plugin" = "xyes"])
> > -
> > + AM_CONDITIONAL(WITH_BSD_NSS, [test "x$bsd_nss" = "xyes"])
> > ])
> >
> > AC_DEFUN([LIBVIRT_RESULT_NSS],[
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index 4320040..6005b8b 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -417,8 +417,22 @@ CLEANFILES += wireshark/src/plugin.c
> >
> > endif WITH_WIRESHARK_DISSECTOR
> >
> > +if WITH_BSD_NSS
> > +LIBVIRT_NSS_SYMBOL_FILE = \
> > + $(srcdir)/nss/libvirt_nss_bsd.syms
> > +NSS_SO_VER = 1
> > +
> > +install-exec-hook:
> > + cd $(DESTDIR)$(libdir) && \
> > + $(LN_S) libnss_libvirt.so.$(NSS_SO_VER) nss_libvirt.so.$(NSS_SO_VER)
> > +
>
> Yeah, very similar link exists on my linux machine too (except for the
> version at EOL). Maybe one day I'll add the link for Linux too.
Ah, so you need that on Linux too? Then we probably make this targets
unconditional?
> > +uninstall-local:
> > + rm $(DESTDIR)$(libdir)/libnss_libvirt.so.$(NSS_SO_VER)
> > +else ! WITH_BSD_NSS
> > LIBVIRT_NSS_SYMBOL_FILE = \
> > $(srcdir)/nss/libvirt_nss.syms
> > +NSS_SO_VER = 2
> > +endif ! WITH_BSD_NSS
> >
> > LIBVIRT_NSS_SOURCES = \
> > nss/libvirt_nss.c \
> > @@ -449,7 +463,7 @@ nss_libnss_libvirt_la_LDFLAGS = \
> > -export-dynamic \
> > -avoid-version \
> > -shared \
> > - -shrext .so.2
> > + -shrext .so.$(NSS_SO_VER)
> >
> > nss_libnss_libvirt_la_LIBADD = \
> > nss/libnss_libvirt_impl.la
> > diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
> > index 218c62a..0ff1348 100644
> > --- a/tools/nss/libvirt_nss.c
> > +++ b/tools/nss/libvirt_nss.c
> > @@ -1,5 +1,4 @@
> > /*
> > - * libvirt_nss: Name Service Switch plugin
>
> This is unintended, right?
Oops, yeah, will get it back.
> > *
> > * The aim is to enable users and applications to translate
> > * domain names into IP addresses. However, this is currently
> > @@ -29,11 +28,16 @@
> >
> > #include "libvirt_nss.h"
> >
> > +#include <netinet/in.h>
> > #include <resolv.h>
> > #include <sys/types.h>
> > #include <dirent.h>
> > #include <arpa/inet.h>
> >
> > +#if defined(HAVE_BSD_NSS)
> > +# include <nsswitch.h>
> > +#endif
> > +
> > #include "virlease.h"
> > #include "viralloc.h"
> > #include "virfile.h"
> > @@ -65,7 +69,7 @@ do { \
> >
> > #define LEASEDIR LOCALSTATEDIR "/lib/libvirt/dnsmasq/"
> >
> > -#define ALIGN(x) (((x) + __SIZEOF_POINTER__ - 1) & ~(__SIZEOF_POINTER__ - 1))
> > +#define LIBVIRT_ALIGN(x) (((x) + __SIZEOF_POINTER__ - 1) & ~(__SIZEOF_POINTER__ - 1))
>
> Yeah, I've seen the name clash with some header file when trying to
> compile on my freebsd machine too.
>
> > #define FAMILY_ADDRESS_SIZE(family) ((family) == AF_INET6 ? 16 : 4)
> >
> > typedef struct {
> > @@ -256,7 +260,7 @@ static inline void *
> > move_and_align(void *buf, size_t len, size_t *idx)
> > {
> > char *buffer = buf;
> > - size_t move = ALIGN(len);
> > + size_t move = LIBVIRT_ALIGN(len);
> >
> > if (!idx)
> > return buffer + move;
> > @@ -321,7 +325,7 @@ _nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result,
> > * b) alias
> > * c) addresses
> > * d) NULL stem */
> > - need = ALIGN(nameLen + 1) + naddr * ALIGN(alen) + (naddr + 2) * sizeof(char*);
> > + need = LIBVIRT_ALIGN(nameLen + 1) + naddr * LIBVIRT_ALIGN(alen) + (naddr + 2) * sizeof(char*);
> >
> > if (buflen < need) {
> > *errnop = ENOMEM;
> > @@ -383,6 +387,7 @@ _nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result,
> > return ret;
> > }
> >
> > +#ifdef HAVE_STRUCT_GAIH_ADDRTUPLE
> > enum nss_status
> > _nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat,
> > char *buffer, size_t buflen, int *errnop,
> > @@ -426,7 +431,7 @@ _nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat,
> > /* We need space for:
> > * a) name
> > * b) addresses */
> > - need = ALIGN(nameLen + 1) + naddr * ALIGN(sizeof(struct gaih_addrtuple));
> > + need = LIBVIRT_ALIGN(nameLen + 1) + naddr * LIBVIRT_ALIGN(sizeof(struct gaih_addrtuple));
> >
> > if (buflen < need) {
> > *errnop = ENOMEM;
> > @@ -474,3 +479,128 @@ _nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat,
> > cleanup:
> > return ret;
> > }
> > +#endif /* HAVE_STRUCT_GAIH_ADDRTUPLE */
>
> I guess this will not fly. I mean, the symbol is exported so I guess we
> need an #else branch providing a dummy implementation, don't we?
> Honestly, I don't know. Maybe we don't as long as we don't run
> check-symfile whether all symbols are exported I guess we are good. Or
> we can just introduce yet another symbol file that would be included
> conditionally.
I just learned about check-symfile. It looks like it's Linux-only and it
looks like it's inside src/Makefile.am, I'm not sure if it checks
tools/.
Also, for BSD I'm using a different sym file that only exposes
nss_module_register().
> BUT, you forgot to include bit that checks for struct gaih_ddrtuple and
> defines HAVE_STRUCT_..?
Right, will get it back.
> > +
> > +#if defined(HAVE_BSD_NSS)
> > +NSS_METHOD_PROTOTYPE(_nss_compat_getaddrinfo);
> > +NSS_METHOD_PROTOTYPE(_nss_compat_gethostbyname2_r);
> > +
> > +ns_mtab methods[] = {
> > + { NSDB_HOSTS, "getaddrinfo", _nss_compat_getaddrinfo, NULL },
> > + { NSDB_HOSTS, "gethostbyname", _nss_compat_gethostbyname2_r, NULL },
> > + { NSDB_HOSTS, "gethostbyname2_r", _nss_compat_gethostbyname2_r, NULL },
> > +};
> > +
> > +static void
> > +aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip)
> > +{
> > + int ret;
> > + struct hostent resolved;
> > + char buf[1024] = { 0 };
> > + int err, herr;
> > + struct addrinfo hints, *res0, *res;
> > + char **addrList;
> > +
> > + if ((ret = _nss_libvirt_gethostbyname2_r(name, af, &resolved,
> > + buf, sizeof(buf),
> > + &err, &herr)) != NS_SUCCESS)
> > + return;
> > +
> > + addrList = resolved.h_addr_list;
> > + while (*addrList) {
> > + virSocketAddr sa;
> > + char *ipAddr = NULL;
> > + void *address = *addrList;
> > +
> > + memset(&sa, 0, sizeof(sa));
> > + if (resolved.h_addrtype == AF_INET) {
> > + virSocketAddrSetIPv4AddrNetOrder(&sa, *((uint32_t *) address));
> > + } else {
> > + virSocketAddrSetIPv6AddrNetOrder(&sa, address);
> > + }
> > +
> > + ipAddr = virSocketAddrFormat(&sa);
> > +
> > + hints = *pai;
> > + hints.ai_flags = AI_NUMERICHOST;
> > + hints.ai_family = af;
> > +
> > + if (getaddrinfo(ipAddr, NULL, &hints, &res0)) {
>
> Interesting. I don't know how NSS works on *bsd but this function is
> called from getaddrinfo() and it is calling getaddrinfo yet again. Is
> that okay?
Apparently it works. I've added some logging for the getaddrinfo()
method and for the helper call as well, and it looks like the inner call
doesn't call the parent method:
https://paste.fedoraproject.org/345046/raw/
I didn't look close at the nss implementation though.
> > + addrList++;
> > + continue;
> > + }
> > +
> > + for (res = res0; res; res = res->ai_next)
> > + res->ai_flags = pai->ai_flags;
> > +
> > + (*aip)->ai_next = res0;
> > + while ((*aip)->ai_next)
> > + *aip = (*aip)->ai_next;
> > +
> > + addrList++;
> > + }
> > +}
> > +
> > +int
> > +_nss_compat_getaddrinfo(void *retval, void *mdata ATTRIBUTE_UNUSED, va_list ap)
> > +{
> > + struct addrinfo sentinel, *cur, *ai;
> > + const char *name;
> > +
>
> Trailing space.
>
> > + name = va_arg(ap, char *);
> > + ai = va_arg(ap, struct addrinfo *);
> > +
> > + memset(&sentinel, 0, sizeof(sentinel));
> > + cur = &sentinel;
> > +
> > + if ((ai->ai_family == AF_UNSPEC) || (ai->ai_family == AF_INET6))
> > + aiforaf(name, AF_INET6, ai, &cur);
> > + if ((ai->ai_family == AF_UNSPEC) || (ai->ai_family == AF_INET))
> > + aiforaf(name, AF_INET, ai, &cur);
> > +
> > + if (sentinel.ai_next == NULL) {
> > + h_errno = HOST_NOT_FOUND;
> > + return NS_NOTFOUND;
> > + }
> > + *((struct addrinfo **)retval) = sentinel.ai_next;
> > +
> > + return NS_SUCCESS;
> > +}
> > +
> > +int
> > +_nss_compat_gethostbyname2_r(void *retval, void *mdata ATTRIBUTE_UNUSED, va_list ap)
> > +{
> > + int ret;
> > +
> > + const char *name;
> > + int af;
> > + struct hostent *result;
> > + char *buffer;
> > + size_t buflen;
> > + int *errnop;
> > + int *herrnop;
> > +
> > + name = va_arg(ap, const char *);
> > + af = va_arg(ap, int);
> > + result = va_arg(ap, struct hostent *);
> > + buffer = va_arg(ap, char *);
> > + buflen = va_arg(ap, size_t);
> > + errnop = va_arg(ap, int *);
> > + herrnop = va_arg(ap, int *);
> > +
> > + ret = _nss_libvirt_gethostbyname2_r(
> > + name, af, result, buffer, buflen, errnop, herrnop);
> > + *(struct hostent **)retval = (ret == NS_SUCCESS) ? result : NULL;
> > +
> > + return ret;
> > +}
> > +
> > +ns_mtab*
> > +nss_module_register(const char *name ATTRIBUTE_UNUSED, unsigned int *size,
> > + nss_module_unregister_fn *unregister)
> > +{
> > + *size = sizeof(methods) / sizeof(methods[0]);
> > + *unregister = NULL;
> > + return methods;
> > +}
> > +#endif /* HAVE_BSD_NSS */
> > diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h
> > index 589c1e6..e025e63 100644
> > --- a/tools/nss/libvirt_nss.h
> > +++ b/tools/nss/libvirt_nss.h
> > @@ -45,8 +45,17 @@ enum nss_status
> > _nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result,
> > char *buffer, size_t buflen, int *errnop,
> > int *herrnop, int32_t *ttlp, char **canonp);
> > +# ifdef HAVE_STRUCT_GAIH_ADDRTUPLE
> > enum nss_status
> > _nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat,
> > char *buffer, size_t buflen, int *errnop,
> > int *herrnop, int32_t *ttlp);
> > +# endif /* HAVE_STRUCT_GAIH_ADDRTUPLE */
> > +
> > +# if defined(HAVE_BSD_NSS)
> > +ns_mtab*
> > +nss_module_register(const char *name, unsigned int *size,
> > + nss_module_unregister_fn *unregister);
> > +# endif /* HAVE_BSD_NSS */
> > +
> > #endif /* __LIBVIRT_NSS_H__ */
> > diff --git a/tools/nss/libvirt_nss_bsd.syms b/tools/nss/libvirt_nss_bsd.syms
> > new file mode 100644
> > index 0000000..7da3926
> > --- /dev/null
> > +++ b/tools/nss/libvirt_nss_bsd.syms
> > @@ -0,0 +1,9 @@
> > +#
> > +# Officially exported symbols.
> > +#
> > +
> > +{
> > +global:
> > + nss_module_register;
> > +local: *;
> > +};
Thanks for review, I hope to roll v2 this weekend. In addition to what
was mentioned I think I'll make nsstests.c not Linux-only too.
>
> Michal
Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list