From: Tom Gundersen Subject: Re: [PATCH v2] rpcbind: add support for systemd socket activation Date: Sat, 4 Feb 2012 09:59:36 +0100 Message-ID: References: <1324602327-1789-1-git-send-email-teg@jklm.no> <1328096830-1383-1-git-send-email-teg@jklm.no> <22B77A95-49D5-4855-A230-A86C57372B28@oracle.com> <59177373-E0FD-4565-94C5-E079C6BE6ED5@oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1559959394==" Cc: Linux NFS Mailing List , "systemd-devel@lists.freedesktop.org" , libtirpc To: Chuck Lever Return-path: In-Reply-To: <59177373-E0FD-4565-94C5-E079C6BE6ED5@oracle.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: systemd-devel-bounces+gcssd-systemd-devel=m.gmane.org@lists.freedesktop.org Errors-To: systemd-devel-bounces+gcssd-systemd-devel=m.gmane.org@lists.freedesktop.org List-ID: --===============1559959394== Content-Type: multipart/alternative; boundary=00504502e2b7192a2804b81fa67a --00504502e2b7192a2804b81fa67a Content-Type: text/plain; charset=UTF-8 On Friday, February 3, 2012, Chuck Lever wrote: > 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 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 >>>> . >>> >>> 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], , > 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? You are right, that's a bug. I'll fix this when I resubmit. Thanks for testing, Tom >> 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 >> #endif >> #include >> +#ifdef SYSTEMD >> +#include >> +#endif >> #include >> #include >> #include >> @@ -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) >> + 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 --00504502e2b7192a2804b81fa67a Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Friday, February 3, 2012, Chuck Lever <chuck.lever@oracle.com> wrote:
> Hi-
&g= t;
> On Feb 1, 2012, at 11:37 AM, Tom Gundersen wrote:
>
>= ;> Hi Chuck,
>>
>> Thanks for the feedback.
>>
>> On We= d, Feb 1, 2012 at 4:32 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
>>> Typically t= his is done by breaking the proposed change into smaller atomic patches. = =C2=A0Is that not possible in this case?
>>
>> Not entirely sure what you have in mind, I don't i= mmediately 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=3D747363>.>>>
>>> Wouldn't comment 3 also work around probl= ems long enough to give us an opportunity to adequately vet the proposed ch= anges?
>>
>> Sure, that's another option.
>>
>&g= t;> 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?
>>
>>>> + =C2=A0 =C2= =A0 /* Try to find if one of the systemd sockets we were given match
>= ;>>> + =C2=A0 =C2=A0 =C2=A0* our netconfig structure. */
>&g= t;>> +
>>>> + =C2=A0 =C2=A0 for (fd =3D SD_LISTEN_FDS_= START; fd < SD_LISTEN_FDS_START + n; fd++) {
>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct __rpc_s= ockinfo si_other;
>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 union {
>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct sockaddr sa;
>>>> + = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struc= t sockaddr_un un;
>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 struct sockaddr_in in4;
>>>> + =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct sockaddr_in6= in6;
>>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 struct sockaddr_storage storage;
>>>&g= t; + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } sa;
>>>
>>> Why is sockaddr_storage included in this union= ?
>>
>> This is from Lennart's original patch. Lennar= t, care to comment?
>>
>> For what it's worth, here i= s the patch with whitespace ignored, which
>> should make it simpler to review:
>>
>>
>&= gt; diff --git a/Makefile.am b/Makefile.am
>> index 9fa608e..194b4= 67 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -38,6 +38,21 @@ rpcbind_SOURCES =3D \
>> =C2=A0 =C2=A0= =C2=A0 src/warmstart.c
>> rpcbind_LDADD =3D $(TIRPC_LIBS)
>= >
>> +if SYSTEMD
>> +AM_CPPFLAGS +=3D $(SYSTEMD_CFLAGS= ) -DSYSTEMD
>> +
>> +rpcbind_LDADD +=3D $(SYSTEMD_LIBS)
>> +
>> +sys= temd/rpcbind.service: systemd/rpcbind= .service.in Makefile
>> + =C2=A0 =C2=A0 sed -e 's,@bindir\= @,$(bindir),g' \
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 < $< > $@ || = rm $@
>> +
>> +systemdsystemunit_DATA =3D \
>> += =C2=A0 =C2=A0 systemd/rpcbind.service \
>> + =C2=A0 =C2=A0 system= d/rpcbind.socket
>> +
>> +endif
>> +
>> rpcinfo_SOURCES =3D =C2=A0 =C2=A0 =C2=A0 src/rpcinfo= .c
>> rpcinfo_LDADD =C2=A0 =3D =C2=A0 =C2=A0 =C2=A0 $(TIRPC_LIBS)<= br>>>
>> diff --git a/config= ure.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([systemdsystemunitdi= r],
>> + =C2=A0AS_HELP_STRING([--with-systemdsystemunitdir=3DDIR],= [Directory for
>> systemd service files]),
>> + =C2=A0[], [with_systemdsyst= emunitdir=3D$($PKG_CONFIG
>> --variable=3Dsystemdsystemunitdir sys= temd)])
>> + =C2=A0if test "x$with_systemdsystemunitdir"= !=3D xno; then
>> + =C2=A0 =C2=A0AC_SUBST([systemdsystemunitdir], [$with_systemdsyst= emunitdir])
>> + =C2=A0 =C2=A0PKG_CHECK_MODULES([SYSTEMD], [libsys= temd-daemon])
>> + =C2=A0fi
>> +AM_CONDITIONAL(SYSTEMD, [= test -n "$with_systemdsystemunitdir" -a
>> "x$with_systemdsystemunitdir" !=3D xno ])
>> +<= br>>> +
>> AS_IF([test x$enable_libwrap =3D xyes], [
>= > =C2=A0 =C2=A0 =C2=A0 AC_CHECK_LIB([wrap], [hosts_access], ,
> It= 's not clear to me how this works on a system that does not have libsys= temd-daemon installed. =C2=A0It appears to require "--with-systemdsyst= emunitdir=3Dno" which a) is not documented that I can see, b) is awkwa= rd ("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 ar= e disabled otherwise. =C2=A0configure options are used to force the situati= on, but out of the shrink wrap, configure should just work.
>
> This way, typically applying a patch does not require any chan= ges to RPMs or other automated build infrastructure except in rare circumst= ances.
>
> Did I miss something?

You are right, that'= ;s a bug. I'll fix this when I resubmit.

Thanks for testing,

Tom


>> diff --git a/src/rpc= bind.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
>> #incl= ude <arpa/inet.h>
>> +#ifdef SYSTEMD
>> +#include &= lt;systemd/sd-daemon.h>
>> +#endif
>> #include <fcn= tl.h>
>> #include <netdb.h>
>> #include <stdio.h>
&= gt;> @@ -281,6 +284,7 @@ init_transport(struct netconfig *nconf)
>= > =C2=A0 =C2=A0 =C2=A0 u_int32_t host_addr[4]; =C2=A0/* IPv4 or IPv6 */<= br>>> =C2=A0 =C2=A0 =C2=A0 struct sockaddr_un sun;
>> =C2=A0 =C2=A0 =C2=A0 mode_t oldmask;
>> + =C2=A0 =C2=A0 i= nt n =3D 0;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 res =3D NULL;
>&g= t;
>> =C2=A0 =C2=A0 =C2=A0 if ((nconf->nc_semantics !=3D NC_TPI= _CLTS) &&
>> @@ -300,6 +304,76 @@ init_transport(struct ne= tconfig *nconf)
>> =C2=A0 =C2=A0 =C2=A0 }
>> #endif
>>
>> = + =C2=A0 =C2=A0 if (!__rpc_nconf2sockinfo(nconf, &si)) {
>> + = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 syslog(LOG_ERR, "cannot get = information for %s",
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 nconf->nc_netid);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (1);
>>= ; + =C2=A0 =C2=A0 }
>> +
>> +#ifdef SYSTEMD
>> += =C2=A0 =C2=A0 n =3D sd_listen_fds(0);
>> + =C2=A0 =C2=A0 if (n &l= t; 0) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 syslog(LOG_= ERR, "failed to acquire systemd scokets: %s", strerror(-n));
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
>> = + =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 /* Try to find = if one of the systemd sockets we were given match
>> + =C2=A0 =C2= =A0 =C2=A0* our netconfig structure. */
>> +
>> + =C2=A0 = =C2=A0 for (fd =3D SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + n; fd= ++) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct __rpc_sockinfo = si_other;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 union {>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 struct sockaddr sa;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct sockaddr_un un;
>> += =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stru= ct sockaddr_in in4;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 struct sockaddr_in6 in6;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct sockaddr_storage storage;<= br>>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } sa;
>> += =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 socklen_t addrlen =3D sizeof(sa)= ;
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!__rpc_fd2sockinfo= (fd, &si_other)) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 syslog(LOG_ERR, "cannot get informatio= n for fd %i", fd);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
>> + =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (si.s= i_af !=3D si_other.si_af ||
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0si.si_socktype !=3D si_other.si_sockt= ype ||
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0si.si_proto !=3D si_other.si_proto)
>> + =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 continue; >> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (gets= ockname(fd, &sa.sa, &addrlen) < 0) = {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 syslog(LOG_ERR, "failed to query socket name: %s",
= >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 strerror(errno));
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 goto error;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = }
>> +
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* = Copy the address */
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= taddr.addr.maxlen =3D taddr.addr.len =3D addrlen;
>> + =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 taddr.addr.buf =3D malloc(addrlen);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (taddr.addr.buf =3D= =3D NULL) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 syslog(LOG_ERR,
>> + =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 "cannot allocate memory for
--00504502e2b7192a2804b81fa67a-- --===============1559959394== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel --===============1559959394==--