On Wed, Dec 11, 2013 at 2:41 AM, Lennart Poettering <lenn...@poettering.net> wrote: > On Wed, 11.12.13 02:13, Daniel Buch (boogiewasth...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel