[libvirt] [PATCH v4 5/8] nss: Implement _nss_libvirt_gethostbyname3_r
Martin Kletzander
mkletzan at redhat.com
Tue Mar 8 15:08:18 UTC 2016
On Thu, Mar 03, 2016 at 06:11:43PM +0100, Michal Privoznik wrote:
>The implementation is pretty straightforward. Moreover, because
>of the nature of things, gethostbyname_r and gethostbyname2_r can
>be implemented at the same time too.
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> config-post.h | 24 ++++
> src/Makefile.am | 59 ++++++++
> src/util/virfile.c | 2 +-
> tests/Makefile.am | 2 +-
> tools/Makefile.am | 8 +-
> tools/nss/libvirt_nss.c | 341 ++++++++++++++++++++++++++++++++++++++++++++-
> tools/nss/libvirt_nss.h | 14 +-
> tools/nss/libvirt_nss.syms | 4 +-
> 8 files changed, 446 insertions(+), 8 deletions(-)
>
>diff --git a/config-post.h b/config-post.h
>index 8367200..2398d3d 100644
>--- a/config-post.h
>+++ b/config-post.h
>@@ -43,3 +43,27 @@
> # undef WITH_YAJL
> # undef WITH_YAJL2
> #endif
>+
>+/*
>+ * With the NSS module it's the same story as virt-login-shell. See the
>+ * explanation above.
>+ */
>+#ifdef LIBVIRT_NSS
>+# undef HAVE_LIBDEVMAPPER_H
>+# undef HAVE_LIBNL
>+# undef HAVE_LIBNL3
>+# undef HAVE_LIBSASL2
>+# undef WITH_CAPNG
>+# undef WITH_CURL
>+# undef WITH_DTRACE_PROBES
>+# undef WITH_GNUTLS
>+# undef WITH_GNUTLS_GCRYPT
>+# undef WITH_MACVTAP
>+# undef WITH_NUMACTL
>+# undef WITH_SASL
>+# undef WITH_SSH2
>+# undef WITH_VIRTUALPORT
>+# undef WITH_SECDRIVER_SELINUX
>+# undef WITH_SECDRIVER_APPARMOR
>+# undef WITH_CAPNG
>+#endif /* LIBVIRT_NSS */
>diff --git a/src/Makefile.am b/src/Makefile.am
>index 2ba397f..8ea5422 100644
>--- a/src/Makefile.am
>+++ b/src/Makefile.am
>@@ -2946,6 +2946,65 @@ endif WITH_LIBVIRTD
> endif WITH_SECDRIVER_APPARMOR
> EXTRA_DIST += $(SECURITY_DRIVER_APPARMOR_HELPER_SOURCES)
>
>+if WITH_NSS
Remove this condition, otherwise you won't be able to build tests
without nss.
>+noinst_LTLIBRARIES += libvirt-nss.la
>+
>+libvirt_nss_la_SOURCES = \
>+ util/viralloc.c \
>+ util/viralloc.h \
>+ util/virbitmap.c \
>+ util/virbitmap.h \
>+ util/virbuffer.c \
>+ util/virbuffer.h \
>+ util/vircommand.c \
>+ util/vircommand.h \
>+ util/virerror.c \
>+ util/virerror.h \
>+ util/virfile.c \
>+ util/virfile.h \
>+ util/virjson.c \
>+ util/virjson.h \
>+ util/virkmod.c \
>+ util/virkmod.h \
>+ util/virlease.c \
>+ util/virlease.h \
>+ util/virlog.c \
>+ util/virlog.h \
>+ util/virobject.c \
>+ util/virobject.h \
>+ util/virpidfile.c \
>+ util/virpidfile.h \
>+ util/virprocess.c \
>+ util/virprocess.h \
>+ util/virsocketaddr.c \
>+ util/virsocketaddr.h \
>+ util/virstring.c \
>+ util/virstring.h \
>+ util/virthread.c \
>+ util/virthread.h \
>+ util/virthreadjob.c \
>+ util/virthreadjob.h \
>+ util/virtime.c \
>+ util/virtime.h \
>+ util/virutil.c \
>+ util/virutil.h \
>+ $(NULL)
>+
>+libvirt_nss_la_CFLAGS = \
>+ -DLIBVIRT_NSS \
>+ $(AM_CFLAGS) \
>+ $(YAJL_CFLAGS) \
>+ $(NULL)
>+libvirt_nss_la_LDFLAGS = \
>+ $(AM_LDFLAGS) \
>+ $(NULL)
>+
>+libvirt_nss_la_LIBADD = \
>+ $(YAJL_LIBS) \
>+ $(NULL)
>+endif WITH_NSS
>+
^^ of course, here too :)
>+
> install-data-local: install-init install-systemd
> if WITH_LIBVIRTD
> $(MKDIR_P) "$(DESTDIR)$(localstatedir)/lib/libvirt/lockd"
>diff --git a/src/util/virfile.c b/src/util/virfile.c
>index f45e18f..0fae0f5 100644
>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -554,7 +554,7 @@ int virFileUpdatePerm(const char *path,
>
>
> #if defined(__linux__) && HAVE_DECL_LO_FLAGS_AUTOCLEAR && \
>- !defined(LIBVIRT_SETUID_RPC_CLIENT)
>+ !defined(LIBVIRT_SETUID_RPC_CLIENT) && !defined(LIBVIRT_NSS)
>
> # if HAVE_DECL_LOOP_CTL_GET_FREE
>
>diff --git a/tests/Makefile.am b/tests/Makefile.am
>index 90981dc..55e8432 100644
>--- a/tests/Makefile.am
>+++ b/tests/Makefile.am
>@@ -65,7 +65,7 @@ GNULIB_LIBS = \
> ../gnulib/lib/libgnu.la
>
> LDADDS = \
>- $(WARN_CFLAGS) \
>+ $(WARN_CFLAGS) \
> $(NO_INDIRECT_LDFLAGS) \
> $(PROBES_O) \
> $(GNULIB_LIBS) \
If I were to nit-pick I would say this has nothing to do here... But
I'm not like that... I won't even mention anything related to that...
>diff --git a/tools/Makefile.am b/tools/Makefile.am
>index 9754e42..8312111 100644
>--- a/tools/Makefile.am
>+++ b/tools/Makefile.am
>@@ -433,7 +433,13 @@ nss_libnss_libvirt_impl_la_CFLAGS = \
> $(AM_CFLAGS) \
> $(WARN_CFLAGS) \
> $(PIE_CFLAGS) \
>- $(COVERAGE_CFLAGS)
>+ $(COVERAGE_CFLAGS) \
>+ $(LIBXML_CFLAGS)
Couldn't we use more #undef's in config-post to get rid of this? It's
not used anywhere in the module implementation.
>+
>+nss_libnss_libvirt_impl_la_LIBADD = \
>+ $(LIBXML_LIBS) \
>+ ../gnulib/lib/libgnu.la \
>+ ../src/libvirt-nss.la
>
> nss_libnss_libvirt_la_SOURCES =
> nss_libnss_libvirt_la_LDFLAGS = \
>diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
>index 461d8ca..192d8e3 100644
>--- a/tools/nss/libvirt_nss.c
>+++ b/tools/nss/libvirt_nss.c
>@@ -29,8 +29,343 @@
>
> #include "libvirt_nss.h"
>
>-int
>-blah(int c)
>+#include <resolv.h>
>+#include <sys/types.h>
>+#include <dirent.h>
>+#include <arpa/inet.h>
>+
>+#include "virlease.h"
>+#include "viralloc.h"
>+#include "virfile.h"
>+#include "virerror.h"
>+#include "virstring.h"
>+#include "virsocketaddr.h"
>+#include "configmake.h"
>+
>+#if 0
>+# define ERROR(...) \
>+do { \
>+ char ebuf[1024]; \
>+ fprintf(stderr, "ERROR %s:%d : ", __FUNCTION__, __LINE__); \
>+ fprintf(stderr, __VA_ARGS__); \
>+ fprintf(stderr, " : %s\n", virStrerror(errno, ebuf, sizeof(ebuf))); \
>+ fprintf(stderr, "\n"); \
>+} while (0)
>+
>+# define DEBUG(...) \
>+do { \
>+ fprintf(stderr, "DEBUG %s:%d : ", __FUNCTION__, __LINE__); \
>+ fprintf(stderr, __VA_ARGS__); \
>+ fprintf(stderr, "\n"); \
>+} while (0)
>+#else
>+# define ERROR(...) do { } while (0)
>+# define DEBUG(...) do { } while (0)
>+#endif
>+
>+#define LEASEDIR LOCALSTATEDIR "/lib/libvirt/dnsmasq/"
>+
>+#define ALIGN4(l) (((l) + 3) & ~3)
>+#define ALIGN8(l) (((l) + 7) & ~7)
>+
>+#if __SIZEOF_POINTER__ == 8
>+# define ALIGN(l) ALIGN8(l)
>+#elif __SIZEOF_POINTER__ == 4
>+# define ALIGN(l) ALIGN4(l)
>+#else
>+# error "Wut? Pointers are neither 4 nor 8 bytes long?"
>+#endif
>+
This is kinda gross. You can avoid needing to do alignment but proper
casts (as I will point out later on so this compiles on clang as well).
But even if you need to go with this, why not do:
#define ALIGN(x) (((x) + __SIZEOF_POINTER__ - 1) & ~(__SIZEOF_POINTER__ - 1))
so that you don't have to create the macro for each size and then select
between them?
>+#define FAMILY_ADDRESS_SIZE(family) ((family) == AF_INET6 ? 16 : 4)
>+
>+typedef struct {
>+ unsigned char addr[16];
>+ int af;
>+} leaseAddress;
>+
>+/**
>+ * findLease:
>+ * @name: domain name to lookup
>+ * @af: address family
>+ * @address: all the addresses found for selected @af
>+ * @naddress: number of elements in @address array
>+ * @found: whether @name has been found
>+ * @errnop: errno pointer
>+ *
>+ * Lookup @name in libvirt's IP database, parse it and store all
>+ * addresses found in @address array. Callers can choose which
>+ * address family (@af) should be returned. Currently only
>+ * AF_INET (IPv4) and AF_INET6 (IPv6) are supported. As a corner
>+ * case, AF_UNSPEC may be passed to @af in which case no address
>+ * filtering is done and addresses from both families are
>+ * returned.
>+ *
>+ * Returns -1 on error
>+ * 0 on success
>+ */
>+static int
>+findLease(const char *name,
>+ int af,
>+ leaseAddress **address,
>+ size_t *naddress,
>+ bool *found,
>+ int *errnop)
> {
>- return c;
>+ DIR *dir = NULL;
>+ int ret = -1;
>+ const char *leaseDir = LEASEDIR;
>+ struct dirent *entry;
>+ virJSONValuePtr leases_array = NULL;
>+ char *server_duid = NULL;
>+ ssize_t i, nleases;
>+ leaseAddress *tmpAddress = NULL;
>+ size_t ntmpAddress = 0;
>+
>+ *address = NULL;
>+ *naddress = 0;
>+ *found = false;
>+
>+ if (af != AF_UNSPEC && af != AF_INET && af != AF_INET6) {
>+ errno = EAFNOSUPPORT;
>+ goto cleanup;
>+ }
>+
>+
>+ if (!(dir = opendir(leaseDir))) {
>+ ERROR("Failed to open dir '%s'", leaseDir);
>+ goto cleanup;
>+ }
>+
>+ if (!(leases_array = virJSONValueNewArray())) {
>+ ERROR("Failed to create json array");
>+ goto cleanup;
>+ }
>+
>+ DEBUG("Dir: %s", leaseDir);
>+ while ((ret = virDirRead(dir, &entry, leaseDir)) > 0) {
>+ char *path;
>+
>+ if (entry->d_name[0] == '.')
>+ continue;
>+
>+ if (!virFileHasSuffix(entry->d_name, ".status"))
>+ continue;
>+
>+ if (!(path = virFileBuildPath(leaseDir, entry->d_name, NULL)))
>+ goto cleanup;
>+
>+ DEBUG("Processing %s", path);
>+
>+ if (virLeaseReadCustomLeaseFile(leases_array, path, NULL, &server_duid) < 0) {
>+ ERROR("Unable to parse %s", path);
>+ VIR_FREE(path);
>+ goto cleanup;
>+ }
>+
>+ VIR_FREE(path);
>+ VIR_FREE(server_duid);
I would rather change virLeaseReadCustomLeaseFile() so that it treats
server_duid == NULL gracefully so that we don't need to do such things
here. But no harm done so far ;)
>+ }
>+
>+ closedir(dir);
>+ dir = NULL;
>+
>+ nleases = virJSONValueArraySize(leases_array);
>+ DEBUG("Read %zd leases", nleases);
>+
>+ for (i = 0; i < nleases; i++) {
>+ virJSONValue const *lease;
>+ const char *lease_name;
>+ virSocketAddr sa;
>+ const char *ipAddr;
>+ int family;
>+
>+ lease = virJSONValueArrayGet(leases_array, i);
>+
>+ if (!lease) {
>+ /* This should never happen (TM) */
>+ ERROR("Unable to get element %zd of %zd", i, nleases);
>+ goto cleanup;
>+ }
>+
>+ lease_name = virJSONValueObjectGetString(lease, "hostname");
>+
>+ if (STRNEQ_NULLABLE(name, lease_name))
>+ continue;
>+
>+ DEBUG("Found record for %s", lease_name);
>+ *found = true;
>+
>+ if (!(ipAddr = virJSONValueObjectGetString(lease, "ip-address"))) {
>+ ERROR("ip-address field missing for %s", name);
>+ goto cleanup;
>+ }
>+
>+ DEBUG("IP address: %s", ipAddr);
>+
>+ if (virSocketAddrParse(&sa, ipAddr, AF_UNSPEC) < 0) {
>+ ERROR("Unable to parse %s", ipAddr);
>+ goto cleanup;
>+ }
>+
>+ family = VIR_SOCKET_ADDR_FAMILY(&sa);
>+ if (af != AF_UNSPEC && af != family) {
>+ DEBUG("Skipping address which family is %d, %d requested", family, af);
>+ continue;
>+ }
>+
>+ if (VIR_REALLOC_N_QUIET(tmpAddress, ntmpAddress + 1) < 0) {
>+ ERROR("Out of memory");
>+ goto cleanup;
>+ }
>+
>+ tmpAddress[ntmpAddress].af = family;
>+ memcpy(tmpAddress[ntmpAddress].addr,
>+ (family == AF_INET ?
>+ (void *) &sa.data.inet4.sin_addr.s_addr :
>+ (void *) &sa.data.inet6.sin6_addr.s6_addr),
>+ FAMILY_ADDRESS_SIZE(family));
>+ ntmpAddress++;
>+ }
>+
>+ *address = tmpAddress;
>+ *naddress = ntmpAddress;
>+ tmpAddress = NULL;
>+ ntmpAddress = 0;
>+
>+ ret = 0;
>+
>+ cleanup:
>+ *errnop = errno;
>+ VIR_FREE(tmpAddress);
>+ virJSONValueFree(leases_array);
>+ if (dir)
>+ closedir(dir);
>+ return ret;
>+}
>+
>+
>+enum nss_status
>+_nss_libvirt_gethostbyname_r(const char *name, struct hostent *result,
>+ char *buffer, size_t buflen, int *errnop,
>+ int *herrnop)
>+{
>+ int af = ((_res.options & RES_USE_INET6) ? AF_INET6 : AF_INET);
>+
>+ return _nss_libvirt_gethostbyname3_r(name, af, result, buffer, buflen,
>+ errnop, herrnop, NULL, NULL);
>+}
>+
>+enum nss_status
>+_nss_libvirt_gethostbyname2_r(const char *name, int af, struct hostent *result,
>+ char *buffer, size_t buflen, int *errnop,
>+ int *herrnop)
>+{
>+ return _nss_libvirt_gethostbyname3_r(name, af, result, buffer, buflen,
>+ errnop, herrnop, NULL, NULL);
>+}
>+
>+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)
>+{
>+ enum nss_status ret = NSS_STATUS_UNAVAIL;
>+ char *r_name, *r_aliases, *r_addr, *r_addr_list;
Most of these point to memory that we don't address by bytes, so they
could be changed to void pointers to indicate that. Or to whatever they
point to so you can use the pointer arithmetic and not that many casts.
>+ leaseAddress *addr = NULL;
>+ size_t naddr, i;
>+ bool found = false;
>+ size_t nameLen, need, idx;
>+ int alen;
>+ int r;
>+
>+ if ((r = findLease(name, af, &addr, &naddr, &found, errnop)) < 0) {
>+ /* Error occurred. Return immediately. */
>+ if (*errnop == EAGAIN) {
>+ *herrnop = TRY_AGAIN;
>+ return NSS_STATUS_TRYAGAIN;
>+ } else {
>+ *herrnop = NO_RECOVERY;
>+ return NSS_STATUS_UNAVAIL;
>+ }
>+ }
>+
>+ if (!found) {
>+ /* NOT found */
>+ *errnop = ESRCH;
>+ *herrnop = HOST_NOT_FOUND;
>+ return NSS_STATUS_NOTFOUND;
>+ } else if (!naddr) {
>+ /* Found, but no data */
>+ *errnop = ENXIO;
>+ *herrnop = NO_DATA;
>+ return NSS_STATUS_UNAVAIL;
>+ }
>+
>+ /* Found and have data */
>+
>+ af = addr[0].af;
>+ alen = FAMILY_ADDRESS_SIZE(af);
>+
So here you base every address' size on the family of the first returned
one. But if there are IPv6 addresses, this function returns random
stuff (tried). I started fixing this and then I realised there were no
links in the commits on how these functions should behave, so I can only
guess how they are supposed to work in case multiple addresses are
returned. I'm guessing the biggest one is used and then it just works
because of the ::<ipv4 address here> ipv6 feature or something like
that, right?
I have some changes for this stored, you might like some of them or
not. They are not split by commit, but see them on my public github and
feel free to incorporate some of it if you like it. At least some
pointers are handled nicely but I haven't rewritten all of the uses.
If you say you only support IPv4 for now, I think we can accept that for
the time being and deal with IPv6 later. But if you want to write the
whole thing, be my guest.
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160308/82d98dc6/attachment-0001.sig>
More information about the libvir-list
mailing list