[systemd-devel] [RFC] Initial libsystemd-asyncns commit
Thomas H.P. Andersen
phomes at gmail.com
Wed Dec 11 14:13:56 PST 2013
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.
>
>> +
>> +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.
It was removed from glib again (see
https://bugzilla.gnome.org/show_bug.cgi?id=616754)
Just before the removal the glib version had this history:
https://git.gnome.org/browse/glib/log/gio/libasyncns/asyncns.c?id=aa586f63543fd584b782dbc1f90bcfae2c96502e
So just non-linux fixes.
>> +#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
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
More information about the systemd-devel
mailing list