Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2014-11-26 Thread Steve Dickson 


On 11/25/2014 12:05 PM, Steve Dickson wrote:
 From: Tom Gundersen t...@jklm.no
 
 Making rpcbind sockect activated will greatly simplify
 its integration in systemd systems. In essence, other services
 may now assume that rpcbind is always available, even during very
 early boot. This means that we no longer need to worry about any
 ordering dependencies.
 
 Original-patch-by: Lennart Poettering lenn...@poettering.net
 Cc: systemd-devel@lists.freedesktop.org
 Acked-by: Cristian Rodríguezcrrodrig...@opensuse.org
 Signed-off-by: Tom Gundersen t...@jklm.no
 Signed-off-by: Steve Dickson ste...@redhat.com
Committed... 

steved.
 ---
  Makefile.am   |  6 +
  configure.ac  | 12 +
  src/rpcbind.c | 81 
 ++-
  3 files changed, 93 insertions(+), 6 deletions(-)
 
 diff --git a/Makefile.am b/Makefile.am
 index 8715082..c99566d 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -41,6 +41,12 @@ rpcbind_SOURCES = \
   src/warmstart.c
  rpcbind_LDADD = $(TIRPC_LIBS)
  
 +if SYSTEMD
 +AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
 +
 +rpcbind_LDADD += $(SYSTEMD_LIBS)
 +endif
 +
  rpcinfo_SOURCES =   src/rpcinfo.c
  rpcinfo_LDADD   =   $(TIRPC_LIBS)
  
 diff --git a/configure.ac b/configure.ac
 index 5a88cc7..967eb05 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -36,6 +36,18 @@ AC_SUBST([nss_modules], [$with_nss_modules])
  
  PKG_CHECK_MODULES([TIRPC], [libtirpc])
  
 +PKG_PROG_PKG_CONFIG
 +AC_ARG_WITH([systemdsystemunitdir],
 +  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
 service files]),
 +  [], [with_systemdsystemunitdir=$($PKG_CONFIG 
 --variable=systemdsystemunitdir systemd)])
 +  if test x$with_systemdsystemunitdir != xno; then
 +AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
 + PKG_CHECK_MODULES([SYSTEMD], [libsystemd], [],
 +[PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon], [],
 +AC_MSG_ERROR([libsystemd support requested but found]))])
 +  fi
 +AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
 x$with_systemdsystemunitdir != xno ])
 +
  AS_IF([test x$enable_libwrap = xyes], [
   AC_CHECK_LIB([wrap], [hosts_access], ,
   AC_MSG_ERROR([libwrap support requested but unable to find 
 libwrap]))
 diff --git a/src/rpcbind.c b/src/rpcbind.c
 index e3462e3..f7c71ee 100644
 --- a/src/rpcbind.c
 +++ b/src/rpcbind.c
 @@ -56,6 +56,9 @@
  #include netinet/in.h
  #endif
  #include arpa/inet.h
 +#ifdef SYSTEMD
 +#include systemd/sd-daemon.h
 +#endif
  #include fcntl.h
  #include netdb.h
  #include stdio.h
 @@ -296,6 +299,7 @@ init_transport(struct netconfig *nconf)
   u_int32_t host_addr[4];  /* IPv4 or IPv6 */
   struct sockaddr_un sun;
   mode_t oldmask;
 + int n;
  res = NULL;
  
   if ((nconf-nc_semantics != NC_TPI_CLTS) 
 @@ -314,6 +318,76 @@ init_transport(struct netconfig *nconf)
   fprintf(stderr, [%d] - %s\n, i, *s);
   }
  #endif
 + if (!__rpc_nconf2sockinfo(nconf, si)) {
 + syslog(LOG_ERR, cannot get information for %s,
 + nconf-nc_netid);
 + return (1);
 + }
 +
 +#ifdef SYSTEMD
 + n = sd_listen_fds(0);
 + if (n  0) {
 + syslog(LOG_ERR, failed to acquire systemd sockets: %s, 
 strerror(-n));
 + return 1;
 + }
 +
 + /* Try to find if one of the systemd sockets we were given match
 +  * our netconfig structure. */
 +
 + for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
 + struct __rpc_sockinfo si_other;
 + union {
 + struct sockaddr sa;
 + struct sockaddr_un un;
 + struct sockaddr_in in4;
 + struct sockaddr_in6 in6;
 + struct sockaddr_storage storage;
 + } sa;
 + socklen_t addrlen = sizeof(sa);
 +
 + if (!__rpc_fd2sockinfo(fd, si_other)) {
 + syslog(LOG_ERR, cannot get information for fd %i, fd);
 + return 1;
 + }
 +
 + if (si.si_af != si_other.si_af ||
 +si.si_socktype != si_other.si_socktype ||
 +si.si_proto != si_other.si_proto)
 + continue;
 +
 + if (getsockname(fd, sa.sa, addrlen)  0) {
 + syslog(LOG_ERR, failed to query socket name: %s,
 +   strerror(errno));
 + goto error;
 + }
 +
 + /* Copy the address */
 + taddr.addr.maxlen = taddr.addr.len = addrlen;
 + taddr.addr.buf = malloc(addrlen);
 + if (taddr.addr.buf == NULL) {
 + syslog(LOG_ERR,
 +   cannot allocate memory for %s address,
 +   nconf-nc_netid);
 + goto error;
 + }
 +

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2014-11-25 Thread Steve Dickson 
Hello,

On 11/22/2014 09:24 PM, Zbigniew Jędrzejewski-Szmek wrote:
 On Fri, Nov 21, 2014 at 11:43:47AM -0500, Steve Dickson wrote:
 From: Tom Gundersen t...@jklm.no

 Making rpcbind sockect activated will greatly simplify
 its integration in systemd systems. In essence, other services
 may now assume that rpcbind is always available, even during very
 early boot. This means that we no longer need to worry about any
 ordering dependencies.

 This is based on a patch originally posted by Lennart Poettering:
 http://permalink.gmane.org/gmane.linux.nfs/33774.

 That patch was not merged due to the lack of a shared library and
 as systemd was seen to be too Fedora specific.

 Systemd now provides a shared library, and it is (or very soon will
 be) the default init system on all the major Linux distributions.

 This version of the patch has three changes from the original:

  * It uses the shared library.
  * It comes with unit files.
  * It is rebased on top of master.

 Please review the patch with git show -b or otherwise ignoring the
 whitespace changes, or it will be extremely difficult to read.
 
 Thanks for working on this... Looks fine to me. Two suggestions
 below.
np... 

 
 v4: reorganized the changes to make the diff easier to read
 remove systemd scripts.

 v3: rebase
 fix typos
 listen on /run/rpcbind.sock, rather than /var/run/rpcbind.sock (the
 latter is a symlink to the former, but this means the socket can be
 created before /var is mounted)
 NB: this version has been compile-tested only as I no longer use
 rpcbind myself
 v2: correctly enable systemd code at compile time
 handle the case where not all the required sockets were supplied
 listen on udp/tcp port 111 in addition to /var/run/rpcbind.sock
 do not daemonize

 Original-patch-by: Lennart Poettering lenn...@poettering.net
 Cc: Steve Dickson ste...@redhat.com
 Cc: systemd-devel@lists.freedesktop.org
 Acked-by: Cristian Rodríguezcrrodrig...@opensuse.org
 Signed-off-by: Tom Gundersen t...@jklm.no
 Signed-off-by: Steve Dickson ste...@redhat.com
 ---
  Makefile.am   |  6 +
  configure.ac  | 10 
  src/rpcbind.c | 81 
 ++-
  3 files changed, 91 insertions(+), 6 deletions(-)

 diff --git a/Makefile.am b/Makefile.am
 index 8715082..c99566d 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -41,6 +41,12 @@ rpcbind_SOURCES = \
  src/warmstart.c
  rpcbind_LDADD = $(TIRPC_LIBS)
  
 +if SYSTEMD
 +AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
 +
 +rpcbind_LDADD += $(SYSTEMD_LIBS)
 +endif
 +
  rpcinfo_SOURCES =   src/rpcinfo.c
  rpcinfo_LDADD   =   $(TIRPC_LIBS)
  
 diff --git a/configure.ac b/configure.ac
 index 5a88cc7..fdad639 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -36,6 +36,16 @@ AC_SUBST([nss_modules], [$with_nss_modules])
  
  PKG_CHECK_MODULES([TIRPC], [libtirpc])
  
 +PKG_PROG_PKG_CONFIG
 +AC_ARG_WITH([systemdsystemunitdir],
 +  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
 service files]),
 +  [], [with_systemdsystemunitdir=$($PKG_CONFIG 
 --variable=systemdsystemunitdir systemd)])
 +  if test x$with_systemdsystemunitdir != xno; then
 +AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
 +PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
 libsystemd-daemon got subsummed by libsystemd. So you should use something
 like
 
PKG_CHECK_MODULES([SYSTEMD], [libsystemd], [],
   [PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon], [],
AC_MSG_ERROR([libsystemd support requested but found]))])
 
 to get the new libary in preference to the old one.
Got it... 

 
 +  fi
 +AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
 x$with_systemdsystemunitdir != xno ])
 +
  AS_IF([test x$enable_libwrap = xyes], [
  AC_CHECK_LIB([wrap], [hosts_access], ,
  AC_MSG_ERROR([libwrap support requested but unable to find 
 libwrap]))
 diff --git a/src/rpcbind.c b/src/rpcbind.c
 index e3462e3..f7c71ee 100644
 --- a/src/rpcbind.c
 +++ b/src/rpcbind.c
 @@ -56,6 +56,9 @@
  #include netinet/in.h
  #endif
  #include arpa/inet.h
 +#ifdef SYSTEMD
 +#include systemd/sd-daemon.h
 +#endif
  #include fcntl.h
  #include netdb.h
  #include stdio.h
 @@ -296,6 +299,7 @@ init_transport(struct netconfig *nconf)
  u_int32_t host_addr[4];  /* IPv4 or IPv6 */
  struct sockaddr_un sun;
  mode_t oldmask;
 +int n;
  res = NULL;
  
  if ((nconf-nc_semantics != NC_TPI_CLTS) 
 @@ -314,6 +318,76 @@ init_transport(struct netconfig *nconf)
  fprintf(stderr, [%d] - %s\n, i, *s);
  }
  #endif
 +if (!__rpc_nconf2sockinfo(nconf, si)) {
 +syslog(LOG_ERR, cannot get information for %s,
 +nconf-nc_netid);
 +return (1);
 +}
 +
 +#ifdef SYSTEMD
 +n = sd_listen_fds(0);
 +if (n  0) {
 +syslog(LOG_ERR, failed to acquire systemd sockets: %s, 
 strerror(-n));
 +return 1;

[systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2014-11-25 Thread Steve Dickson 
From: Tom Gundersen t...@jklm.no

Making rpcbind sockect activated will greatly simplify
its integration in systemd systems. In essence, other services
may now assume that rpcbind is always available, even during very
early boot. This means that we no longer need to worry about any
ordering dependencies.

Original-patch-by: Lennart Poettering lenn...@poettering.net
Cc: systemd-devel@lists.freedesktop.org
Acked-by: Cristian Rodríguezcrrodrig...@opensuse.org
Signed-off-by: Tom Gundersen t...@jklm.no
Signed-off-by: Steve Dickson ste...@redhat.com
---
 Makefile.am   |  6 +
 configure.ac  | 12 +
 src/rpcbind.c | 81 ++-
 3 files changed, 93 insertions(+), 6 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 8715082..c99566d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -41,6 +41,12 @@ rpcbind_SOURCES = \
src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)
 
+if SYSTEMD
+AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
+
+rpcbind_LDADD += $(SYSTEMD_LIBS)
+endif
+
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)
 
diff --git a/configure.ac b/configure.ac
index 5a88cc7..967eb05 100644
--- a/configure.ac
+++ b/configure.ac
@@ -36,6 +36,18 @@ AC_SUBST([nss_modules], [$with_nss_modules])
 
 PKG_CHECK_MODULES([TIRPC], [libtirpc])
 
+PKG_PROG_PKG_CONFIG
+AC_ARG_WITH([systemdsystemunitdir],
+  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
service files]),
+  [], [with_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir 
systemd)])
+  if test x$with_systemdsystemunitdir != xno; then
+AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
+ PKG_CHECK_MODULES([SYSTEMD], [libsystemd], [],
+  [PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon], [],
+  AC_MSG_ERROR([libsystemd support requested but found]))])
+  fi
+AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
x$with_systemdsystemunitdir != xno ])
+
 AS_IF([test x$enable_libwrap = xyes], [
AC_CHECK_LIB([wrap], [hosts_access], ,
AC_MSG_ERROR([libwrap support requested but unable to find 
libwrap]))
diff --git a/src/rpcbind.c b/src/rpcbind.c
index e3462e3..f7c71ee 100644
--- a/src/rpcbind.c
+++ b/src/rpcbind.c
@@ -56,6 +56,9 @@
 #include netinet/in.h
 #endif
 #include arpa/inet.h
+#ifdef SYSTEMD
+#include systemd/sd-daemon.h
+#endif
 #include fcntl.h
 #include netdb.h
 #include stdio.h
@@ -296,6 +299,7 @@ init_transport(struct netconfig *nconf)
u_int32_t host_addr[4];  /* IPv4 or IPv6 */
struct sockaddr_un sun;
mode_t oldmask;
+   int n;
 res = NULL;
 
if ((nconf-nc_semantics != NC_TPI_CLTS) 
@@ -314,6 +318,76 @@ init_transport(struct netconfig *nconf)
fprintf(stderr, [%d] - %s\n, i, *s);
}
 #endif
+   if (!__rpc_nconf2sockinfo(nconf, si)) {
+   syslog(LOG_ERR, cannot get information for %s,
+   nconf-nc_netid);
+   return (1);
+   }
+
+#ifdef SYSTEMD
+   n = sd_listen_fds(0);
+   if (n  0) {
+   syslog(LOG_ERR, failed to acquire systemd sockets: %s, 
strerror(-n));
+   return 1;
+   }
+
+   /* Try to find if one of the systemd sockets we were given match
+* our netconfig structure. */
+
+   for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
+   struct __rpc_sockinfo si_other;
+   union {
+   struct sockaddr sa;
+   struct sockaddr_un un;
+   struct sockaddr_in in4;
+   struct sockaddr_in6 in6;
+   struct sockaddr_storage storage;
+   } sa;
+   socklen_t addrlen = sizeof(sa);
+
+   if (!__rpc_fd2sockinfo(fd, si_other)) {
+   syslog(LOG_ERR, cannot get information for fd %i, fd);
+   return 1;
+   }
+
+   if (si.si_af != si_other.si_af ||
+si.si_socktype != si_other.si_socktype ||
+si.si_proto != si_other.si_proto)
+   continue;
+
+   if (getsockname(fd, sa.sa, addrlen)  0) {
+   syslog(LOG_ERR, failed to query socket name: %s,
+   strerror(errno));
+   goto error;
+   }
+
+   /* Copy the address */
+   taddr.addr.maxlen = taddr.addr.len = addrlen;
+   taddr.addr.buf = malloc(addrlen);
+   if (taddr.addr.buf == NULL) {
+   syslog(LOG_ERR,
+   cannot allocate memory for %s address,
+   nconf-nc_netid);
+   goto error;
+   }
+   memcpy(taddr.addr.buf, sa, addrlen);
+
+   my_xprt = (SVCXPRT *)svc_tli_create(fd, nconf, taddr,
+

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2014-11-22 Thread Zbigniew Jędrzejewski-Szmek 
On Fri, Nov 21, 2014 at 11:43:47AM -0500, Steve Dickson wrote:
 From: Tom Gundersen t...@jklm.no
 
 Making rpcbind sockect activated will greatly simplify
 its integration in systemd systems. In essence, other services
 may now assume that rpcbind is always available, even during very
 early boot. This means that we no longer need to worry about any
 ordering dependencies.
 
 This is based on a patch originally posted by Lennart Poettering:
 http://permalink.gmane.org/gmane.linux.nfs/33774.
 
 That patch was not merged due to the lack of a shared library and
 as systemd was seen to be too Fedora specific.
 
 Systemd now provides a shared library, and it is (or very soon will
 be) the default init system on all the major Linux distributions.
 
 This version of the patch has three changes from the original:
 
  * It uses the shared library.
  * It comes with unit files.
  * It is rebased on top of master.
 
 Please review the patch with git show -b or otherwise ignoring the
 whitespace changes, or it will be extremely difficult to read.

Thanks for working on this... Looks fine to me. Two suggestions
below.

 v4: reorganized the changes to make the diff easier to read
 remove systemd scripts.
 
 v3: rebase
 fix typos
 listen on /run/rpcbind.sock, rather than /var/run/rpcbind.sock (the
 latter is a symlink to the former, but this means the socket can be
 created before /var is mounted)
 NB: this version has been compile-tested only as I no longer use
 rpcbind myself
 v2: correctly enable systemd code at compile time
 handle the case where not all the required sockets were supplied
 listen on udp/tcp port 111 in addition to /var/run/rpcbind.sock
 do not daemonize
 
 Original-patch-by: Lennart Poettering lenn...@poettering.net
 Cc: Steve Dickson ste...@redhat.com
 Cc: systemd-devel@lists.freedesktop.org
 Acked-by: Cristian Rodríguezcrrodrig...@opensuse.org
 Signed-off-by: Tom Gundersen t...@jklm.no
 Signed-off-by: Steve Dickson ste...@redhat.com
 ---
  Makefile.am   |  6 +
  configure.ac  | 10 
  src/rpcbind.c | 81 
 ++-
  3 files changed, 91 insertions(+), 6 deletions(-)
 
 diff --git a/Makefile.am b/Makefile.am
 index 8715082..c99566d 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -41,6 +41,12 @@ rpcbind_SOURCES = \
   src/warmstart.c
  rpcbind_LDADD = $(TIRPC_LIBS)
  
 +if SYSTEMD
 +AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
 +
 +rpcbind_LDADD += $(SYSTEMD_LIBS)
 +endif
 +
  rpcinfo_SOURCES =   src/rpcinfo.c
  rpcinfo_LDADD   =   $(TIRPC_LIBS)
  
 diff --git a/configure.ac b/configure.ac
 index 5a88cc7..fdad639 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -36,6 +36,16 @@ AC_SUBST([nss_modules], [$with_nss_modules])
  
  PKG_CHECK_MODULES([TIRPC], [libtirpc])
  
 +PKG_PROG_PKG_CONFIG
 +AC_ARG_WITH([systemdsystemunitdir],
 +  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
 service files]),
 +  [], [with_systemdsystemunitdir=$($PKG_CONFIG 
 --variable=systemdsystemunitdir systemd)])
 +  if test x$with_systemdsystemunitdir != xno; then
 +AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
 +PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
libsystemd-daemon got subsummed by libsystemd. So you should use something
like

   PKG_CHECK_MODULES([SYSTEMD], [libsystemd], [],
  [PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon], [],
   AC_MSG_ERROR([libsystemd support requested but found]))])

to get the new libary in preference to the old one.

 +  fi
 +AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
 x$with_systemdsystemunitdir != xno ])
 +
  AS_IF([test x$enable_libwrap = xyes], [
   AC_CHECK_LIB([wrap], [hosts_access], ,
   AC_MSG_ERROR([libwrap support requested but unable to find 
 libwrap]))
 diff --git a/src/rpcbind.c b/src/rpcbind.c
 index e3462e3..f7c71ee 100644
 --- a/src/rpcbind.c
 +++ b/src/rpcbind.c
 @@ -56,6 +56,9 @@
  #include netinet/in.h
  #endif
  #include arpa/inet.h
 +#ifdef SYSTEMD
 +#include systemd/sd-daemon.h
 +#endif
  #include fcntl.h
  #include netdb.h
  #include stdio.h
 @@ -296,6 +299,7 @@ init_transport(struct netconfig *nconf)
   u_int32_t host_addr[4];  /* IPv4 or IPv6 */
   struct sockaddr_un sun;
   mode_t oldmask;
 + int n;
  res = NULL;
  
   if ((nconf-nc_semantics != NC_TPI_CLTS) 
 @@ -314,6 +318,76 @@ init_transport(struct netconfig *nconf)
   fprintf(stderr, [%d] - %s\n, i, *s);
   }
  #endif
 + if (!__rpc_nconf2sockinfo(nconf, si)) {
 + syslog(LOG_ERR, cannot get information for %s,
 + nconf-nc_netid);
 + return (1);
 + }
 +
 +#ifdef SYSTEMD
 + n = sd_listen_fds(0);
 + if (n  0) {
 + syslog(LOG_ERR, failed to acquire systemd sockets: %s, 
 strerror(-n));
 + return 1;
 + }
 +
 + /* Try to find if one of the systemd sockets we were

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-03 Thread Chuck Lever 
Hi-

On Feb 1, 2012, at 11:37 AM, Tom Gundersen wrote:

 Hi Chuck,
 
 Thanks for the feedback.
 
 On Wed, Feb 1, 2012 at 4:32 PM, Chuck Lever chuck.le...@oracle.com wrote:
 Typically this is done by breaking the proposed change into smaller atomic 
 patches.  Is that not possible in this case?
 
 Not entirely sure what you have in mind, I don't immediately see how
 to split this up. Any suggestions welcome.
 
 Added Michal to cc as this patch should go a long way to sort out
 https://bugzilla.redhat.com/show_bug.cgi?id=747363.
 
 Wouldn't comment 3 also work around problems long enough to give us an 
 opportunity to adequately vet the proposed changes?
 
 Sure, that's another option.
 
 What additions to our test matrix are needed?
 
 I could not find any tests in the rpcbind git repo. Could you point me
 at your test matrix?
 
 + /* Try to find if one of the systemd sockets we were given match
 +  * our netconfig structure. */
 +
 + for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
 + struct __rpc_sockinfo si_other;
 + union {
 + struct sockaddr sa;
 + struct sockaddr_un un;
 + struct sockaddr_in in4;
 + struct sockaddr_in6 in6;
 + struct sockaddr_storage storage;
 + } sa;
 
 Why is sockaddr_storage included in this union?
 
 This is from Lennart's original patch. Lennart, care to comment?
 
 For what it's worth, here is the patch with whitespace ignored, which
 should make it simpler to review:
 
 
 diff --git a/Makefile.am b/Makefile.am
 index 9fa608e..194b467 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -38,6 +38,21 @@ rpcbind_SOURCES = \
   src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)
 
 +if SYSTEMD
 +AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
 +
 +rpcbind_LDADD += $(SYSTEMD_LIBS)
 +
 +systemd/rpcbind.service: systemd/rpcbind.service.in Makefile
 + sed -e 's,@bindir\@,$(bindir),g' \
 +  $  $@ || rm $@
 +
 +systemdsystemunit_DATA = \
 + systemd/rpcbind.service \
 + systemd/rpcbind.socket
 +
 +endif
 +
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)
 
 diff --git a/configure.in b/configure.in
 index 2b67720..397d52d 100644
 --- a/configure.in
 +++ b/configure.in
 @@ -29,6 +29,17 @@ AC_SUBST([rpcuser], [$with_rpcuser])
 
 PKG_CHECK_MODULES([TIRPC], [libtirpc])
 
 +PKG_PROG_PKG_CONFIG
 +AC_ARG_WITH([systemdsystemunitdir],
 +  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for
 systemd service files]),
 +  [], [with_systemdsystemunitdir=$($PKG_CONFIG
 --variable=systemdsystemunitdir systemd)])
 +  if test x$with_systemdsystemunitdir != xno; then
 +AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
 +PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
 +  fi
 +AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a
 x$with_systemdsystemunitdir != xno ])
 +
 +
 AS_IF([test x$enable_libwrap = xyes], [
   AC_CHECK_LIB([wrap], [hosts_access], ,
   AC_MSG_ERROR([libwrap support requested but unable to find 
 libwrap]))

It's not clear to me how this works on a system that does not have 
libsystemd-daemon installed.  It appears to require 
--with-systemdsystemunitdir=no which a) is not documented that I can see, b) 
is awkward (no is not a directory name), and c) violates the principal of 
least surprise.

Our usual practice is to add features so they are enabled when they find all of 
the dependencies already on the build system, and they are disabled otherwise.  
configure options are used to force the situation, but out of the shrink wrap, 
configure should just work.

This way, typically applying a patch does not require any changes to RPMs or 
other automated build infrastructure except in rare circumstances.

Did I miss something?

 diff --git a/src/rpcbind.c b/src/rpcbind.c
 index 24e069b..a87ce05 100644
 --- a/src/rpcbind.c
 +++ b/src/rpcbind.c
 @@ -56,6 +56,9 @@
 #include netinet/in.h
 #endif
 #include arpa/inet.h
 +#ifdef SYSTEMD
 +#include systemd/sd-daemon.h
 +#endif
 #include fcntl.h
 #include netdb.h
 #include stdio.h
 @@ -281,6 +284,7 @@ init_transport(struct netconfig *nconf)
   u_int32_t host_addr[4];  /* IPv4 or IPv6 */
   struct sockaddr_un sun;
   mode_t oldmask;
 + int n = 0;
 res = NULL;
 
   if ((nconf-nc_semantics != NC_TPI_CLTS) 
 @@ -300,6 +304,76 @@ init_transport(struct netconfig *nconf)
   }
 #endif
 
 + if (!__rpc_nconf2sockinfo(nconf, si)) {
 + syslog(LOG_ERR, cannot get information for %s,
 + nconf-nc_netid);
 + return (1);
 + }
 +
 +#ifdef SYSTEMD
 + n = sd_listen_fds(0);
 + if (n  0) {
 + syslog(LOG_ERR, failed to acquire systemd scokets: %s, 
 strerror(-n));
 + return 1;
 + }
 +
 + /* Try to find if one of the systemd sockets we were given match
 +  * our netconfig structure.

[systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Tom Gundersen 
Making rpcbind sockect activated will greatly simplify
its integration in systemd systems. In essence, other services
may now assume that rpcbind is always available, even during very
early boot. This means that we no longer need to worry about any
ordering dependencies.

This is based on a patch originally posted by Lennart Poettering:
http://permalink.gmane.org/gmane.linux.nfs/33774.

That patch was not merged due to the lack of a shared library and
as systemd was seen to be too Fedora specific.

Systemd now provides a shared library, and it is shipped by defalt in
OpenSUSE in addition to Fedora, and it is available in Debain, Gentoo,
Arch, and others.

This version of the patch has three changes from the original:

 * It uses the shared library.
 * It comes with unit files.
 * It is rebased on top of master.

Please review the patch with git show -b or otherwise ignoring the
whitespace changes, or it will be extremely difficult to read.

Comments welcome.

v2: correctly enable systemd code at compile time
handle the case where not all the required sockets were supplied
listen on udp/tcp port 111 in addition to /var/run/rpcbind.sock
do not daemonize

Original-patch-by: Lennart Poettering lenn...@poettering.net
Cc: Michal Schmidt mschm...@redhat.com
Cc: Steve Dickson ste...@redhat.com
Cc: systemd-devel@lists.freedesktop.org
Acked-by: Cristian Rodríguezcrrodrig...@opensuse.org
Signed-off-by: Tom Gundersen t...@jklm.no
---

What's the status on this?

Lennart, does your ack still appl after my changes?

Steve, any chance of applying this?

If this is applied I have a couple of follow ups. In particular, I'd like to
do the cleanup of init_transport that Jim suggested as a separate patch,
as leaving the line-wraps alone makes this patch easier to read I think.

Added Michal to cc as this patch should go a long way to sort out
https://bugzilla.redhat.com/show_bug.cgi?id=747363.

Cheers,

Tom

 Makefile.am|   15 ++
 configure.in   |   11 +
 src/rpcbind.c  |  467 +---
 systemd/.gitignore |1 +
 systemd/rpcbind.service.in |9 +
 systemd/rpcbind.socket |   12 ++
 6 files changed, 316 insertions(+), 199 deletions(-)
 create mode 100644 systemd/.gitignore
 create mode 100644 systemd/rpcbind.service.in
 create mode 100644 systemd/rpcbind.socket

diff --git a/Makefile.am b/Makefile.am
index 9fa608e..194b467 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -38,6 +38,21 @@ rpcbind_SOURCES = \
src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)
 
+if SYSTEMD
+AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
+
+rpcbind_LDADD += $(SYSTEMD_LIBS)
+
+systemd/rpcbind.service: systemd/rpcbind.service.in Makefile
+   sed -e 's,@bindir\@,$(bindir),g' \
+$  $@ || rm $@
+
+systemdsystemunit_DATA = \
+   systemd/rpcbind.service \
+   systemd/rpcbind.socket
+
+endif
+
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)
 
diff --git a/configure.in b/configure.in
index 2b67720..397d52d 100644
--- a/configure.in
+++ b/configure.in
@@ -29,6 +29,17 @@ AC_SUBST([rpcuser], [$with_rpcuser])
  
 PKG_CHECK_MODULES([TIRPC], [libtirpc])
 
+PKG_PROG_PKG_CONFIG
+AC_ARG_WITH([systemdsystemunitdir],
+  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
service files]),
+  [], [with_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir 
systemd)])
+  if test x$with_systemdsystemunitdir != xno; then
+AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
+PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
+  fi
+AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
x$with_systemdsystemunitdir != xno ])
+
+
 AS_IF([test x$enable_libwrap = xyes], [
AC_CHECK_LIB([wrap], [hosts_access], ,
AC_MSG_ERROR([libwrap support requested but unable to find 
libwrap]))
diff --git a/src/rpcbind.c b/src/rpcbind.c
index 24e069b..a87ce05 100644
--- a/src/rpcbind.c
+++ b/src/rpcbind.c
@@ -56,6 +56,9 @@
 #include netinet/in.h
 #endif
 #include arpa/inet.h
+#ifdef SYSTEMD
+#include systemd/sd-daemon.h
+#endif
 #include fcntl.h
 #include netdb.h
 #include stdio.h
@@ -281,6 +284,7 @@ init_transport(struct netconfig *nconf)
u_int32_t host_addr[4];  /* IPv4 or IPv6 */
struct sockaddr_un sun;
mode_t oldmask;
+   int n = 0;
 res = NULL;
 
if ((nconf-nc_semantics != NC_TPI_CLTS) 
@@ -300,141 +304,285 @@ init_transport(struct netconfig *nconf)
}
 #endif
 
-   /*
-* XXX - using RPC library internal functions. For NC_TPI_CLTS
-* we call this later, for each socket we like to bind.
-*/
-   if (nconf-nc_semantics != NC_TPI_CLTS) {
-   if ((fd = __rpc_nconf2fd(nconf))  0) {
-   syslog(LOG_ERR, cannot create socket for %s,
-   nconf-nc_netid);
-   return (1);
-   }
-   }
-

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Zbigniew Jędrzejewski-Szmek 

Hi,
exactly what version of rpcbind is this supposed to be applied against?
(Even better, could you post a link to a git repo?)

Confused,
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Tom Gundersen 
Hi Zbyszek,

2012/2/1 Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl:
 exactly what version of rpcbind is this supposed to be applied against?

Current git head.

 (Even better, could you post a link to a git repo?)

Sure, the patch can be found on my wip branch here:
https://github.com/teg/rpcbind/commits/wip. Though, note that the
subsequent patches on that branch are not meant for public consumption
quite yet :-)

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Chuck Lever 
Adding libtirpc developers mailing list...

On Feb 1, 2012, at 6:47 AM, Tom Gundersen wrote:

 Making rpcbind sockect activated will greatly simplify
 its integration in systemd systems. In essence, other services
 may now assume that rpcbind is always available, even during very
 early boot. This means that we no longer need to worry about any
 ordering dependencies.
 
 This is based on a patch originally posted by Lennart Poettering:
 http://permalink.gmane.org/gmane.linux.nfs/33774.
 
 That patch was not merged due to the lack of a shared library and
 as systemd was seen to be too Fedora specific.
 
 Systemd now provides a shared library, and it is shipped by defalt in
 OpenSUSE in addition to Fedora, and it is available in Debain, Gentoo,
 Arch, and others.
 
 This version of the patch has three changes from the original:
 
 * It uses the shared library.
 * It comes with unit files.
 * It is rebased on top of master.
 
 Please review the patch with git show -b or otherwise ignoring the
 whitespace changes, or it will be extremely difficult to read.
 
 Comments welcome.
 
 v2: correctly enable systemd code at compile time
handle the case where not all the required sockets were supplied
listen on udp/tcp port 111 in addition to /var/run/rpcbind.sock
do not daemonize
 
 Original-patch-by: Lennart Poettering lenn...@poettering.net
 Cc: Michal Schmidt mschm...@redhat.com
 Cc: Steve Dickson ste...@redhat.com
 Cc: systemd-devel@lists.freedesktop.org
 Acked-by: Cristian Rodríguezcrrodrig...@opensuse.org
 Signed-off-by: Tom Gundersen t...@jklm.no
 ---
 
 What's the status on this?
 
 Lennart, does your ack still appl after my changes?
 
 Steve, any chance of applying this?
 
 If this is applied I have a couple of follow ups. In particular, I'd like to
 do the cleanup of init_transport that Jim suggested as a separate patch,
 as leaving the line-wraps alone makes this patch easier to read I think.

Typically this is done by breaking the proposed change into smaller atomic 
patches.  Is that not possible in this case?

I can play with this more later today or tomorrow.

 Added Michal to cc as this patch should go a long way to sort out
 https://bugzilla.redhat.com/show_bug.cgi?id=747363.

Wouldn't comment 3 also work around problems long enough to give us an 
opportunity to adequately vet the proposed changes?

What additions to our test matrix are needed?

One more immediate comment below.

 Cheers,
 
 Tom
 
 Makefile.am|   15 ++
 configure.in   |   11 +
 src/rpcbind.c  |  467 +---
 systemd/.gitignore |1 +
 systemd/rpcbind.service.in |9 +
 systemd/rpcbind.socket |   12 ++
 6 files changed, 316 insertions(+), 199 deletions(-)
 create mode 100644 systemd/.gitignore
 create mode 100644 systemd/rpcbind.service.in
 create mode 100644 systemd/rpcbind.socket
 
 diff --git a/Makefile.am b/Makefile.am
 index 9fa608e..194b467 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -38,6 +38,21 @@ rpcbind_SOURCES = \
   src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)
 
 +if SYSTEMD
 +AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
 +
 +rpcbind_LDADD += $(SYSTEMD_LIBS)
 +
 +systemd/rpcbind.service: systemd/rpcbind.service.in Makefile
 + sed -e 's,@bindir\@,$(bindir),g' \
 +  $  $@ || rm $@
 +
 +systemdsystemunit_DATA = \
 + systemd/rpcbind.service \
 + systemd/rpcbind.socket
 +
 +endif
 +
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)
 
 diff --git a/configure.in b/configure.in
 index 2b67720..397d52d 100644
 --- a/configure.in
 +++ b/configure.in
 @@ -29,6 +29,17 @@ AC_SUBST([rpcuser], [$with_rpcuser])
 
 PKG_CHECK_MODULES([TIRPC], [libtirpc])
 
 +PKG_PROG_PKG_CONFIG
 +AC_ARG_WITH([systemdsystemunitdir],
 +  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
 service files]),
 +  [], [with_systemdsystemunitdir=$($PKG_CONFIG 
 --variable=systemdsystemunitdir systemd)])
 +  if test x$with_systemdsystemunitdir != xno; then
 +AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
 +PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
 +  fi
 +AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
 x$with_systemdsystemunitdir != xno ])
 +
 +
 AS_IF([test x$enable_libwrap = xyes], [
   AC_CHECK_LIB([wrap], [hosts_access], ,
   AC_MSG_ERROR([libwrap support requested but unable to find 
 libwrap]))
 diff --git a/src/rpcbind.c b/src/rpcbind.c
 index 24e069b..a87ce05 100644
 --- a/src/rpcbind.c
 +++ b/src/rpcbind.c
 @@ -56,6 +56,9 @@
 #include netinet/in.h
 #endif
 #include arpa/inet.h
 +#ifdef SYSTEMD
 +#include systemd/sd-daemon.h
 +#endif
 #include fcntl.h
 #include netdb.h
 #include stdio.h
 @@ -281,6 +284,7 @@ init_transport(struct netconfig *nconf)
   u_int32_t host_addr[4];  /* IPv4 or IPv6 */
   struct sockaddr_un sun;
   mode_t oldmask;
 + int n = 0;
 res = NULL;
 
   if

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Tom Gundersen 
Hi Chuck,

Thanks for the feedback.

On Wed, Feb 1, 2012 at 4:32 PM, Chuck Lever chuck.le...@oracle.com wrote:
 Typically this is done by breaking the proposed change into smaller atomic 
 patches.  Is that not possible in this case?

Not entirely sure what you have in mind, I don't immediately see how
to split this up. Any suggestions welcome.

 Added Michal to cc as this patch should go a long way to sort out
 https://bugzilla.redhat.com/show_bug.cgi?id=747363.

 Wouldn't comment 3 also work around problems long enough to give us an 
 opportunity to adequately vet the proposed changes?

Sure, that's another option.

 What additions to our test matrix are needed?

I could not find any tests in the rpcbind git repo. Could you point me
at your test matrix?

 +     /* Try to find if one of the systemd sockets we were given match
 +      * our netconfig structure. */
 +
 +     for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
 +             struct __rpc_sockinfo si_other;
 +             union {
 +                     struct sockaddr sa;
 +                     struct sockaddr_un un;
 +                     struct sockaddr_in in4;
 +                     struct sockaddr_in6 in6;
 +                     struct sockaddr_storage storage;
 +             } sa;

 Why is sockaddr_storage included in this union?

This is from Lennart's original patch. Lennart, care to comment?

For what it's worth, here is the patch with whitespace ignored, which
should make it simpler to review:


diff --git a/Makefile.am b/Makefile.am
index 9fa608e..194b467 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -38,6 +38,21 @@ rpcbind_SOURCES = \
src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)

+if SYSTEMD
+AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
+
+rpcbind_LDADD += $(SYSTEMD_LIBS)
+
+systemd/rpcbind.service: systemd/rpcbind.service.in Makefile
+   sed -e 's,@bindir\@,$(bindir),g' \
+$  $@ || rm $@
+
+systemdsystemunit_DATA = \
+   systemd/rpcbind.service \
+   systemd/rpcbind.socket
+
+endif
+
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)

diff --git a/configure.in b/configure.in
index 2b67720..397d52d 100644
--- a/configure.in
+++ b/configure.in
@@ -29,6 +29,17 @@ AC_SUBST([rpcuser], [$with_rpcuser])

 PKG_CHECK_MODULES([TIRPC], [libtirpc])

+PKG_PROG_PKG_CONFIG
+AC_ARG_WITH([systemdsystemunitdir],
+  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for
systemd service files]),
+  [], [with_systemdsystemunitdir=$($PKG_CONFIG
--variable=systemdsystemunitdir systemd)])
+  if test x$with_systemdsystemunitdir != xno; then
+AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
+PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
+  fi
+AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a
x$with_systemdsystemunitdir != xno ])
+
+
 AS_IF([test x$enable_libwrap = xyes], [
AC_CHECK_LIB([wrap], [hosts_access], ,
AC_MSG_ERROR([libwrap support requested but unable to find 
libwrap]))
diff --git a/src/rpcbind.c b/src/rpcbind.c
index 24e069b..a87ce05 100644
--- a/src/rpcbind.c
+++ b/src/rpcbind.c
@@ -56,6 +56,9 @@
 #include netinet/in.h
 #endif
 #include arpa/inet.h
+#ifdef SYSTEMD
+#include systemd/sd-daemon.h
+#endif
 #include fcntl.h
 #include netdb.h
 #include stdio.h
@@ -281,6 +284,7 @@ init_transport(struct netconfig *nconf)
u_int32_t host_addr[4];  /* IPv4 or IPv6 */
struct sockaddr_un sun;
mode_t oldmask;
+   int n = 0;
 res = NULL;

if ((nconf-nc_semantics != NC_TPI_CLTS) 
@@ -300,6 +304,76 @@ init_transport(struct netconfig *nconf)
}
 #endif

+   if (!__rpc_nconf2sockinfo(nconf, si)) {
+   syslog(LOG_ERR, cannot get information for %s,
+   nconf-nc_netid);
+   return (1);
+   }
+
+#ifdef SYSTEMD
+   n = sd_listen_fds(0);
+   if (n  0) {
+   syslog(LOG_ERR, failed to acquire systemd scokets: %s, 
strerror(-n));
+   return 1;
+   }
+
+   /* Try to find if one of the systemd sockets we were given match
+* our netconfig structure. */
+
+   for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
+   struct __rpc_sockinfo si_other;
+   union {
+   struct sockaddr sa;
+   struct sockaddr_un un;
+   struct sockaddr_in in4;
+   struct sockaddr_in6 in6;
+   struct sockaddr_storage storage;
+   } sa;
+   socklen_t addrlen = sizeof(sa);
+
+   if (!__rpc_fd2sockinfo(fd, si_other)) {
+   syslog(LOG_ERR, cannot get information for fd %i, fd);
+   return 1;
+   }
+
+   if (si.si_af != si_other.si_af ||
+si.si_socktype != si_other.si_socktype ||
+si.si_proto != si_other.si_proto)
+

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Chuck Lever 

On Feb 1, 2012, at 11:37 AM, Tom Gundersen wrote:

 Hi Chuck,
 
 Thanks for the feedback.
 
 On Wed, Feb 1, 2012 at 4:32 PM, Chuck Lever chuck.le...@oracle.com wrote:
 Typically this is done by breaking the proposed change into smaller atomic 
 patches.  Is that not possible in this case?
 
 Not entirely sure what you have in mind, I don't immediately see how
 to split this up. Any suggestions welcome.

Submit the patch you have below, then white space fixes become subsequent clean 
up patches.  Not only is review easier now, but if we come back to these 
changes in a year to fix bugs, it's easier for us to re-learn what it does.

 
 Added Michal to cc as this patch should go a long way to sort out
 https://bugzilla.redhat.com/show_bug.cgi?id=747363.
 
 Wouldn't comment 3 also work around problems long enough to give us an 
 opportunity to adequately vet the proposed changes?
 
 Sure, that's another option.
 
 What additions to our test matrix are needed?
 
 I could not find any tests in the rpcbind git repo. Could you point me
 at your test matrix?

I'll state the question differently: When the SYSTEMD macro is defined, what 
kind of tests should we run? In general, how do we confirm this is working as 
expected?  (There may be no good answer at the moment).

 
 + /* Try to find if one of the systemd sockets we were given match
 +  * our netconfig structure. */
 +
 + for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
 + struct __rpc_sockinfo si_other;
 + union {
 + struct sockaddr sa;
 + struct sockaddr_un un;
 + struct sockaddr_in in4;
 + struct sockaddr_in6 in6;
 + struct sockaddr_storage storage;
 + } sa;
 
 Why is sockaddr_storage included in this union?
 
 This is from Lennart's original patch. Lennart, care to comment?
 
 For what it's worth, here is the patch with whitespace ignored, which
 should make it simpler to review:
 
 
 diff --git a/Makefile.am b/Makefile.am
 index 9fa608e..194b467 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -38,6 +38,21 @@ rpcbind_SOURCES = \
   src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)
 
 +if SYSTEMD
 +AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
 +
 +rpcbind_LDADD += $(SYSTEMD_LIBS)
 +
 +systemd/rpcbind.service: systemd/rpcbind.service.in Makefile
 + sed -e 's,@bindir\@,$(bindir),g' \
 +  $  $@ || rm $@
 +
 +systemdsystemunit_DATA = \
 + systemd/rpcbind.service \
 + systemd/rpcbind.socket
 +
 +endif
 +
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)
 
 diff --git a/configure.in b/configure.in
 index 2b67720..397d52d 100644
 --- a/configure.in
 +++ b/configure.in
 @@ -29,6 +29,17 @@ AC_SUBST([rpcuser], [$with_rpcuser])
 
 PKG_CHECK_MODULES([TIRPC], [libtirpc])
 
 +PKG_PROG_PKG_CONFIG
 +AC_ARG_WITH([systemdsystemunitdir],
 +  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for
 systemd service files]),
 +  [], [with_systemdsystemunitdir=$($PKG_CONFIG
 --variable=systemdsystemunitdir systemd)])
 +  if test x$with_systemdsystemunitdir != xno; then
 +AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
 +PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
 +  fi
 +AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a
 x$with_systemdsystemunitdir != xno ])
 +
 +
 AS_IF([test x$enable_libwrap = xyes], [
   AC_CHECK_LIB([wrap], [hosts_access], ,
   AC_MSG_ERROR([libwrap support requested but unable to find 
 libwrap]))
 diff --git a/src/rpcbind.c b/src/rpcbind.c
 index 24e069b..a87ce05 100644
 --- a/src/rpcbind.c
 +++ b/src/rpcbind.c
 @@ -56,6 +56,9 @@
 #include netinet/in.h
 #endif
 #include arpa/inet.h
 +#ifdef SYSTEMD
 +#include systemd/sd-daemon.h
 +#endif
 #include fcntl.h
 #include netdb.h
 #include stdio.h
 @@ -281,6 +284,7 @@ init_transport(struct netconfig *nconf)
   u_int32_t host_addr[4];  /* IPv4 or IPv6 */
   struct sockaddr_un sun;
   mode_t oldmask;
 + int n = 0;
 res = NULL;
 
   if ((nconf-nc_semantics != NC_TPI_CLTS) 
 @@ -300,6 +304,76 @@ init_transport(struct netconfig *nconf)
   }
 #endif
 
 + if (!__rpc_nconf2sockinfo(nconf, si)) {
 + syslog(LOG_ERR, cannot get information for %s,
 + nconf-nc_netid);
 + return (1);
 + }
 +
 +#ifdef SYSTEMD
 + n = sd_listen_fds(0);
 + if (n  0) {
 + syslog(LOG_ERR, failed to acquire systemd scokets: %s, 
 strerror(-n));
 + return 1;
 + }
 +
 + /* Try to find if one of the systemd sockets we were given match
 +  * our netconfig structure. */
 +
 + for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
 + struct __rpc_sockinfo si_other;
 + union {
 + struct sockaddr sa;
 + struct sockaddr_un un;
 + struct sockaddr_in in4;

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Tom Gundersen 
On Wed, Feb 1, 2012 at 7:16 PM, Chuck Lever chuck.le...@oracle.com wrote:
 Submit the patch you have below, then white space fixes become subsequent 
 clean up patches.  Not only is review easier now, but if we come back to 
 these changes in a year to fix bugs, it's easier for us to re-learn what it 
 does.

I'll resubmit as two patches (one only doing indentation). Though,
I'll wait a ittle while in case there is more feedback.

 I'll state the question differently: When the SYSTEMD macro is defined, what 
 kind of tests should we run? In general, how do we confirm this is working as 
 expected?  (There may be no good answer at the moment).

In general the patch is meant to achieve two things:

1) If not using systemd, everything should work as before, even if the
SYSTEMD macro is defined.

2) When using systemd, enabling rpcbind.socket should cause rpcbind to
start on demand and act as if it was running all along. Anything that
used to be After=rpcbind.service should simply be
After=rpcbind.socket (or more generally: After=rpcbind.target,
which in turn is After=rpcbind.socket).

Cheers,

Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Chuck Lever 
Hi-

On Feb 1, 2012, at 6:47 AM, Tom Gundersen wrote:

 Making rpcbind sockect activated will greatly simplify
 its integration in systemd systems. In essence, other services
 may now assume that rpcbind is always available, even during very
 early boot. This means that we no longer need to worry about any
 ordering dependencies.
 
 This is based on a patch originally posted by Lennart Poettering:
 http://permalink.gmane.org/gmane.linux.nfs/33774.
 
 That patch was not merged due to the lack of a shared library and
 as systemd was seen to be too Fedora specific.
 
 Systemd now provides a shared library, and it is shipped by defalt in
 OpenSUSE in addition to Fedora, and it is available in Debain, Gentoo,
 Arch, and others.
 
 This version of the patch has three changes from the original:
 
 * It uses the shared library.
 * It comes with unit files.
 * It is rebased on top of master.
 
 Please review the patch with git show -b or otherwise ignoring the
 whitespace changes, or it will be extremely difficult to read.
 
 Comments welcome.
 
 v2: correctly enable systemd code at compile time
handle the case where not all the required sockets were supplied
listen on udp/tcp port 111 in addition to /var/run/rpcbind.sock
do not daemonize
 
 Original-patch-by: Lennart Poettering lenn...@poettering.net
 Cc: Michal Schmidt mschm...@redhat.com
 Cc: Steve Dickson ste...@redhat.com
 Cc: systemd-devel@lists.freedesktop.org
 Acked-by: Cristian Rodríguezcrrodrig...@opensuse.org
 Signed-off-by: Tom Gundersen t...@jklm.no
 ---
 
 What's the status on this?
 
 Lennart, does your ack still appl after my changes?
 
 Steve, any chance of applying this?
 
 If this is applied I have a couple of follow ups. In particular, I'd like to
 do the cleanup of init_transport that Jim suggested as a separate patch,
 as leaving the line-wraps alone makes this patch easier to read I think.
 
 Added Michal to cc as this patch should go a long way to sort out
 https://bugzilla.redhat.com/show_bug.cgi?id=747363.
 
 Cheers,
 
 Tom
 
 Makefile.am|   15 ++
 configure.in   |   11 +
 src/rpcbind.c  |  467 +---
 systemd/.gitignore |1 +
 systemd/rpcbind.service.in |9 +
 systemd/rpcbind.socket |   12 ++
 6 files changed, 316 insertions(+), 199 deletions(-)
 create mode 100644 systemd/.gitignore
 create mode 100644 systemd/rpcbind.service.in
 create mode 100644 systemd/rpcbind.socket
 
 diff --git a/Makefile.am b/Makefile.am
 index 9fa608e..194b467 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -38,6 +38,21 @@ rpcbind_SOURCES = \
   src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)
 
 +if SYSTEMD
 +AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
 +
 +rpcbind_LDADD += $(SYSTEMD_LIBS)
 +
 +systemd/rpcbind.service: systemd/rpcbind.service.in Makefile
 + sed -e 's,@bindir\@,$(bindir),g' \
 +  $  $@ || rm $@
 +
 +systemdsystemunit_DATA = \
 + systemd/rpcbind.service \
 + systemd/rpcbind.socket
 +
 +endif
 +
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)
 
 diff --git a/configure.in b/configure.in
 index 2b67720..397d52d 100644
 --- a/configure.in
 +++ b/configure.in

If I do a make distclean I don't have a configure.in.  Should you be patching 
configure.ac instead?

 @@ -29,6 +29,17 @@ AC_SUBST([rpcuser], [$with_rpcuser])
 
 PKG_CHECK_MODULES([TIRPC], [libtirpc])
 
 +PKG_PROG_PKG_CONFIG
 +AC_ARG_WITH([systemdsystemunitdir],
 +  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
 service files]),
 +  [], [with_systemdsystemunitdir=$($PKG_CONFIG 
 --variable=systemdsystemunitdir systemd)])
 +  if test x$with_systemdsystemunitdir != xno; then
 +AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
 +PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
 +  fi
 +AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
 x$with_systemdsystemunitdir != xno ])
 +
 +
 AS_IF([test x$enable_libwrap = xyes], [
   AC_CHECK_LIB([wrap], [hosts_access], ,
   AC_MSG_ERROR([libwrap support requested but unable to find 
 libwrap]))
 

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com




___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Lennart Poettering 
On Wed, 01.02.12 17:37, Tom Gundersen (t...@jklm.no) wrote:

  +     /* Try to find if one of the systemd sockets we were given match
  +      * our netconfig structure. */
  +
  +     for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
  +             struct __rpc_sockinfo si_other;
  +             union {
  +                     struct sockaddr sa;
  +                     struct sockaddr_un un;
  +                     struct sockaddr_in in4;
  +                     struct sockaddr_in6 in6;
  +                     struct sockaddr_storage storage;
  +             } sa;
 
  Why is sockaddr_storage included in this union?
 
 This is from Lennart's original patch. Lennart, care to comment?

Well, simply because sockaddr_storage is the actual struct one should
normally use for this kind of thing (where you try to determine a
sockaddr from a socket you don't know at all). With one exception
however, sockaddr_un is actually longer than sockaddr_storage, which is
documented borkedness in the socket API.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2012-02-01 Thread Chuck Lever 

On Feb 1, 2012, at 4:45 PM, Lennart Poettering wrote:

 On Wed, 01.02.12 17:37, Tom Gundersen (t...@jklm.no) wrote:
 
 + /* Try to find if one of the systemd sockets we were given match
 +  * our netconfig structure. */
 +
 + for (fd = SD_LISTEN_FDS_START; fd  SD_LISTEN_FDS_START + n; fd++) {
 + struct __rpc_sockinfo si_other;
 + union {
 + struct sockaddr sa;
 + struct sockaddr_un un;
 + struct sockaddr_in in4;
 + struct sockaddr_in6 in6;
 + struct sockaddr_storage storage;
 + } sa;
 
 Why is sockaddr_storage included in this union?
 
 This is from Lennart's original patch. Lennart, care to comment?
 
 Well, simply because sockaddr_storage is the actual struct one should
 normally use for this kind of thing (where you try to determine a
 sockaddr from a socket you don't know at all). With one exception
 however, sockaddr_un is actually longer than sockaddr_storage, which is
 documented borkedness in the socket API.

You don't reference any of the fields inside this union, except for sa.  It 
seems unnecessary to include all of these members, and then not use most of 
them.

The biggest address you're ever going to get out of libtirpc is a 
sockaddr_storage.  If we must stick with a union to prevent pointer aliasing, 
can we have just two members: sockaddr and sockaddr_storage?

Otherwise, there's no need for this kind of generality here:  TI-RPC handles 
only IPv4, IPv6, and AF_LOCAL.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com




___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2011-12-22 Thread Tom Gundersen 
Making rpcbind sockect activated will greatly simplify
its integration in systemd systems. In essence, other services
may now assume that rpcbind is always available, even during very
early boot. This means that we no longer need to worry about any
ordering dependencies.

This is based on a patch originally posted by Lennart Poettering:
http://permalink.gmane.org/gmane.linux.nfs/33774.

That patch was not merged due to the lack of a shared library and
as systemd was seen to be too Fedora specific.

Systemd now provides a shared library, and it is shipped by defalt in
OpenSUSE in addition to Fedora, and it is available in Debain, Gentoo,
Arch, and others.

This version of the patch has three changes from the original:

 * It uses the shared library.
 * It comes with unit files.
 * It is rebased on top of master.

Please review the patch with git show -b or otherwise ignoring the
whitespace changes, or it will be extremely difficult to read.

Comments welcome.

v2: correctly enable systemd code at compile time
handle the case where not all the required sockets were supplied
listen on udp/tcp port 111 in addition to /var/run/rpcbind.sock
do not daemonize

Original-patch-by: Lennart Poettering lenn...@poettering.net
Cc: Steve Dickson ste...@redhat.com
Cc: systemd-devel@lists.freedesktop.org
Cc: Cristian Rodríguez crrodrig...@opensuse.org
Signed-off-by: Tom Gundersen t...@jklm.no
---

Thanks to Cristian for testing. The testcase I had been using was entirely 
flawed,
the code did in fact not work at all. Sorry about that!

This time around it should work :-)

 Makefile.am|   15 ++
 configure.in   |   11 +
 src/rpcbind.c  |  467 +---
 systemd/.gitignore |1 +
 systemd/rpcbind.service.in |9 +
 systemd/rpcbind.socket |   12 ++
 6 files changed, 316 insertions(+), 199 deletions(-)
 create mode 100644 systemd/.gitignore
 create mode 100644 systemd/rpcbind.service.in
 create mode 100644 systemd/rpcbind.socket

diff --git a/Makefile.am b/Makefile.am
index 9fa608e..194b467 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -38,6 +38,21 @@ rpcbind_SOURCES = \
src/warmstart.c
 rpcbind_LDADD = $(TIRPC_LIBS)
 
+if SYSTEMD
+AM_CPPFLAGS += $(SYSTEMD_CFLAGS) -DSYSTEMD
+
+rpcbind_LDADD += $(SYSTEMD_LIBS)
+
+systemd/rpcbind.service: systemd/rpcbind.service.in Makefile
+   sed -e 's,@bindir\@,$(bindir),g' \
+$  $@ || rm $@
+
+systemdsystemunit_DATA = \
+   systemd/rpcbind.service \
+   systemd/rpcbind.socket
+
+endif
+
 rpcinfo_SOURCES =   src/rpcinfo.c
 rpcinfo_LDADD   =   $(TIRPC_LIBS)
 
diff --git a/configure.in b/configure.in
index 2b67720..397d52d 100644
--- a/configure.in
+++ b/configure.in
@@ -29,6 +29,17 @@ AC_SUBST([rpcuser], [$with_rpcuser])
  
 PKG_CHECK_MODULES([TIRPC], [libtirpc])
 
+PKG_PROG_PKG_CONFIG
+AC_ARG_WITH([systemdsystemunitdir],
+  AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd 
service files]),
+  [], [with_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir 
systemd)])
+  if test x$with_systemdsystemunitdir != xno; then
+AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
+PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon])
+  fi
+AM_CONDITIONAL(SYSTEMD, [test -n $with_systemdsystemunitdir -a 
x$with_systemdsystemunitdir != xno ])
+
+
 AS_IF([test x$enable_libwrap = xyes], [
AC_CHECK_LIB([wrap], [hosts_access], ,
AC_MSG_ERROR([libwrap support requested but unable to find 
libwrap]))
diff --git a/src/rpcbind.c b/src/rpcbind.c
index 24e069b..a87ce05 100644
--- a/src/rpcbind.c
+++ b/src/rpcbind.c
@@ -56,6 +56,9 @@
 #include netinet/in.h
 #endif
 #include arpa/inet.h
+#ifdef SYSTEMD
+#include systemd/sd-daemon.h
+#endif
 #include fcntl.h
 #include netdb.h
 #include stdio.h
@@ -281,6 +284,7 @@ init_transport(struct netconfig *nconf)
u_int32_t host_addr[4];  /* IPv4 or IPv6 */
struct sockaddr_un sun;
mode_t oldmask;
+   int n = 0;
 res = NULL;
 
if ((nconf-nc_semantics != NC_TPI_CLTS) 
@@ -300,141 +304,285 @@ init_transport(struct netconfig *nconf)
}
 #endif
 
-   /*
-* XXX - using RPC library internal functions. For NC_TPI_CLTS
-* we call this later, for each socket we like to bind.
-*/
-   if (nconf-nc_semantics != NC_TPI_CLTS) {
-   if ((fd = __rpc_nconf2fd(nconf))  0) {
-   syslog(LOG_ERR, cannot create socket for %s,
-   nconf-nc_netid);
-   return (1);
-   }
-   }
-
if (!__rpc_nconf2sockinfo(nconf, si)) {
syslog(LOG_ERR, cannot get information for %s,
nconf-nc_netid);
return (1);
}
 
-   if ((strcmp(nconf-nc_netid, local) == 0) ||
-   (strcmp(nconf-nc_netid, unix) == 0)) {
-   memset(sun, 0, sizeof sun);
-

Re: [systemd-devel] [PATCH] rpcbind: add support for systemd socket activation

2011-12-22 Thread Cristian Rodríguez 

On 22/12/11 22:05, Tom Gundersen wrote:

Making rpcbind sockect activated will greatly simplify
its integration in systemd systems. In essence, other services
may now assume that rpcbind is always available, even during very
early boot. This means that we no longer need to worry about any
ordering dependencies.

This is based on a patch originally posted by Lennart Poettering:
http://permalink.gmane.org/gmane.linux.nfs/33774.

That patch was not merged due to the lack of a shared library and
as systemd was seen to be too Fedora specific.

Systemd now provides a shared library, and it is shipped by defalt in
OpenSUSE in addition to Fedora, and it is available in Debain, Gentoo,
Arch, and others.

This version of the patch has three changes from the original:

  * It uses the shared library.
  * It comes with unit files.
  * It is rebased on top of master.

Please review the patch with git show -b or otherwise ignoring the
whitespace changes, or it will be extremely difficult to read.

Comments welcome.

v2: correctly enable systemd code at compile time
 handle the case where not all the required sockets were supplied
 listen on udp/tcp port 111 in addition to /var/run/rpcbind.sock
 do not daemonize

Original-patch-by: Lennart Poetteringlenn...@poettering.net
Cc: Steve Dicksonste...@redhat.com
Cc: systemd-devel@lists.freedesktop.org
Cc: Cristian Rodríguezcrrodrig...@opensuse.org
Signed-off-by: Tom Gundersent...@jklm.no
---

Thanks to Cristian for testing. The testcase I had been using was entirely 
flawed,
the code did in fact not work at all. Sorry about that!


ACKed: Cristian Rodríguezcrrodrig...@opensuse.org

This version works as expected here.


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel