[systemd-devel] [RFC] Initial libsystemd-asyncns commit

[Lennart Poettering]

On Wed, 11.12.13 02:13, Daniel Buch (boogiewasthere at gmail.com) wrote:

Heya,

Hmm, so thinking about it I have the suspicion this should probably be
linked into libsystemd-bus, and thus live in src/libsystemd-bus/. The
reason for this is cyclic deps: libsystemd-bus really should make use of
this to resolve host names when used in tcp mode. However, this stuff
really should come with integration into sd-event out of the box too
which is already part of libsystemd-bus (for similar reasons). So we'd
have asyncns both as consumer of APIs of libsystemd-bus and as
provider of APIs to it. 

Of course, one could argue that DNS is hardly part of bus access but
then again, sd-event is neither. Maybe we should just start treating
libsystemd-bus as that library where everything we expose ends up in,
except when it is really clear that it's only consumer, never provider
to stuff in libsystemd-bus.

> Reindentation is done to fit systemd
> ---
>  Makefile.am                           |   23 +
>  src/libsystemd-asyncns/asyncns.c      | 1513 +++++++++++++++++++++++++++++++++
>  src/libsystemd-asyncns/asyncns.h      |  163 ++++
>  src/libsystemd-asyncns/test-asyncns.c |  178 ++++
>  4 files changed, 1877 insertions(+)
>  create mode 100644 src/libsystemd-asyncns/asyncns.c
>  create mode 100644 src/libsystemd-asyncns/asyncns.h
>  create mode 100644 src/libsystemd-asyncns/test-asyncns.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 19da6ea..a0564b5 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -659,6 +659,29 @@ tests += test-rtnl
>  
>  # ------------------------------------------------------------------------------
>  noinst_LTLIBRARIES += \
> +	libsystemd-asyncns.la
> +
> +libsystemd_asyncns_la_SOURCES = \
> +	src/libsystemd-asyncns/asyncns.c \
> +	src/libsystemd-asyncns/asyncns.h

To follow the naming scheme of the other libs we should probably call
this "sd-asyncs.c", and the header file should be in src/systemd/
together with the other public APIs.

> +
> +libsystemd_asyncns_la_CFLAGS = \
> +	-pthread
> +
> +test_asyncns_SOURCES = \
> +	src/libsystemd-asyncns/test-asyncns.c
> +
> +test_asyncns_LDADD = \
> +	libsystemd-asyncns.la
> +
> +test_asyncns_LDFLAGS = \
> +	-lresolv \
> +	-pthread

I figure the -lresolv thing will eventually need a configure.ac check,
but for now I'd just leave it like this, when this breaks for people I
am sure they will cook up a patch quickly...

> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif

Unnecessary, we do this on the gcc cmdline implicitly far all our files...

> +
> +/* #undef HAVE_PTHREAD */
> +
> +#include <assert.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <sys/select.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <sys/wait.h>
> +#include <sys/types.h>
> +#include <pwd.h>
> +#include <netinet/in.h>
> +#include <arpa/nameser.h>
> +#include <resolv.h>
> +#include <dirent.h>
> +#include <sys/time.h>
> +#include <sys/resource.h>
> +#include <stdint.h>
> +
> +#ifdef HAVE_SYS_PRCTL_H
> +#include <sys/prctl.h>
> +#endif

This file isn't optional for us...

> +
> +#if HAVE_PTHREAD
> +#include <pthread.h>
> +#endif
> +
> +#include "asyncns.h"
> +
> +#ifndef MSG_NOSIGNAL
> +#define MSG_NOSIGNAL 0
> +#endif

This is non-Linux compat and should go i figure...

> +struct asyncns {

probably should be called sd_asyncns now...

> +        int fds[MESSAGE_FD_MAX];
> +
> +#ifndef HAVE_PTHREAD
> +        pid_t workers[MAX_WORKERS];
> +#else
> +        pthread_t workers[MAX_WORKERS];
> +#endif

We should probably stick to one implementation here... Either with
process or with threads, but not chicken out anymore like I did
before...

Every other day I come to different conclusions which one would be the
one and only mode we should support. 

The big advantage of the process option is that things are neatly
isolated from the parent. THe big disadvantages are that it pollutes the
"ps" output and might generate page faults in the parent all the time,
since we fork and don't exec().

Right now I am more leaning towards the thread option. But maybe that
changes tomorrow again. Other opinions? Kay?

glib is embedding a copy of libasyncns currently, they opted for the
thread solution. Maybe we should follow suit.

(Actually, it might be worth checking the glib copy, and see if they
made any changes to the sources, which we might want to steal back for us.

> +#ifndef HAVE_PTHREAD
> +
> +static int close_allv(const int except_fds[]) {

We already have this in close_many() in util.h. This function can go away without replacement.

> +static int reset_sigsv(const int except[]) {

This too in reset_all_signal_handlers().

> +static int fd_nonblock(int fd) {

And this we have tooo.

> +static int fd_cloexec(int fd) {

And this too...

> +
> +        switch (req->type) {
> +
> +                case REQUEST_ADDRINFO: {
> +                                               struct addrinfo ai, *result = NULL;
> +                                               const addrinfo_request_t *ai_req = &packet->addrinfo_request;
> +                                               const char *node, *service;
> +                                               int ret;

There's something really wrong with the indentation here....

Should be:

switch (req->type) {

case REQUEST_ADDRINFO: {
        struct addrinfo ai, *result = NULL;
        ...
}

...

}
> +        assert(in_fd > 2);
> +        assert(out_fd > 2);
> +
> +        close(0);
> +        close(1);
> +        close(2);
> +
> +        if (open("/dev/null", O_RDONLY) != 0)
> +                goto fail;
> +
> +        if (open("/dev/null", O_WRONLY) != 1)
> +                goto fail;
> +
> +        if (open("/dev/null", O_WRONLY) != 2)
> +                goto fail;

We have a call for the above  in make_null_stdio() already.

Lennart

-- 
Lennart Poettering, Red Hat