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

[Tom Gundersen]

On Thu, Jan 2, 2014 at 10:53 PM, Tom Gundersen <teg at jklm.no> wrote:
> On Wed, Dec 11, 2013 at 2:41 AM, Lennart Poettering
> <lennart at poettering.net> wrote:
>> 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.
>
> Hm, the fact that this is async is sort of a detail, is it not? I
> mean, all our libs are async... How about sd-dns? It is slightly
> wrong, but sd-ns could mean anything...
>
>>> +
>>> +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];
>>> +#elseso, you just suggested pthreads is the way to go. any strong reasons?
>>> +        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.
>
> Doesn't seem to be any strong arguments either way... So shall we just
> go with pthreads, if that's what's used elsewhere...?

Kay says:
> yeah, a thread. seems natural, we do not share data with the main process really
> robusteness should not be an argument, for something rather trivial

-t