Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Thu, Sep 10, Ian Jackson wrote: > Wei Liu writes ("Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to > start xenstored"): > > On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote: > > > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS > > > > And the difference between these two approaches is /usr/bin/env > > preserves envars while sh -c doesn't? > > Who interpolates $XENSTORED and $XENSTORED_ARGS here ? Was systemd > doing that all along ? > If XENSTORED_ARGS contains spaces, what happens ? Yes, its described in the docs. Also $var vs. ${var} is slightly different: http://www.freedesktop.org/software/systemd/man/systemd.service.html ... Command lines ... Basic environment variable substitution is supported. Use "${FOO}" as part of a word, or as a word of its own, on the command line, in which case it will be replaced by the value of the environment variable including all whitespace it contains, resulting in a single argument. Use "$FOO" as a separate word on the command line, in which case it will be replaced by the value of the environment variable split at whitespace resulting in zero or more arguments. For this type of expansion, quotes and respected when splitting into words, and afterwards removed. ... Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
Wei Liu writes ("Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored"): > On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote: > > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS > > And the difference between these two approaches is /usr/bin/env > preserves envars while sh -c doesn't? Who interpolates $XENSTORED and $XENSTORED_ARGS here ? Was systemd doing that all along ? If XENSTORED_ARGS contains spaces, what happens ? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Thu, Sep 10, 2015 at 4:01 PM, M A Young wrote: > On Thu, 10 Sep 2015, Wei Liu wrote: > >> On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote: >> > On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering wrote: >> > > On Tue, Jan 06, Ian Campbell wrote: >> > > >> > >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote: >> > >> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. >> > >> > >> > >> > Create a separate wrapper script which is used in the sysv runlevel >> > >> > script and in systemd xenstored.service. It preserves existing >> > >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements >> > >> > the handling of XENSTORED_ARGS=. This variable has to be added to >> > >> > sysconfig/xencommons. >> > >> >> > >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an >> > >> example in the sysconfig file of enabling tracing if you like)? >> > > >> > > After having two weeks to think about this I came to the same >> > > conclusion. I think whatever the outcome is, the boolean should be >> > > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a >> > > help text which mentions "-T /path" and "xenstored --help" to get other >> > > options because there is no man page. >> > > >> > >> Going to a wrapper script just to make some fairly uncommon debugging >> > >> option marginally more convenient seems like overkill to me, plus >> > >> XENSTORED_ARGS would allow for passing other useful options to >> > >> xenstored. >> > > >> > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt >> > > was to expand the XENSTORE variable from the sysconfig file. But this >> > > approach leads to failures with SELinux because the socket passing does >> > > not work this way. Up to now I have not seen a success report for >> > > selinux+systemd+xenstored. Maybe its already somewhere in the other >> > > unread mails. >> > > >> > > In my cover letter I provided some possible ways to handle >> > > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env >> > > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its >> > > possible to select a binary via the sysconfig file. But it also means >> > > the XENSTORE_TRACE boolean has to be removed in favour of the plain >> > > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the >> > > need for a wrapper script. >> > > >> > > Hopefully someone with access to a SELinux enabled system will report >> > > which approach actually works. >> > >> > I can confirm that >> > >> > ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS" >> > >> > does not work on a CentOS7 system with selinux enabled, but that >> > >> > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS >> > >> >> And the difference between these two approaches is /usr/bin/env >> preserves envars while sh -c doesn't? > > /bin/sh doesn't give the right selinux permissions so xenstored doesn't > start. Presumably /usr/bin/env does. Or to be more precise, /bin/sh exec from that context doesn't allow xenstored to make the accept() system call, so you get a log full of these: type=PROCTITLE msg=audit(1441892166.173:22242): proctitle=2F7573722F7362696E2F78656E73746F726564002D2D6E6F2D666F726B type=AVC msg=audit(1441892166.173:22243): avc: denied { accept } for pid=615 comm="xenstored" path="/run/xenstored/socket" scontext=system_u:system_r:xenstored_t:s0 tcontext=system_u:system_r:initrc_t:s0 tclass=unix_stream_socket permissive=0 type=SYSCALL msg=audit(1441892166.173:22243): arch=c03e syscall=43 success=no exit=-13 a0=3 a1=0 a2=0 a3=0 items=0 ppid=1 pid=615 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="xenstored" exe="/usr/sbin/xenstored" subj=system_u:system_r:xenstored_t:s0 key=(null) At least, I assume the denied {accept} means the accept system call... I haven't verified that syscall 43 is in fact accept. I assume that "env" since was designed to execute other programs, selinux on CentOS 7 has been programmed to retain the context. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Thu, Sep 10, 2015 at 04:01:06PM +0100, M A Young wrote: > On Thu, 10 Sep 2015, Wei Liu wrote: > > > On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote: > > > On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering wrote: > > > > On Tue, Jan 06, Ian Campbell wrote: > > > > > > > >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote: > > > >> > The shell wrapper in xenstored.service does not handle > > > >> > XENSTORE_TRACE. > > > >> > > > > >> > Create a separate wrapper script which is used in the sysv runlevel > > > >> > script and in systemd xenstored.service. It preserves existing > > > >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements > > > >> > the handling of XENSTORED_ARGS=. This variable has to be added to > > > >> > sysconfig/xencommons. > > > >> > > > >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an > > > >> example in the sysconfig file of enabling tracing if you like)? > > > > > > > > After having two weeks to think about this I came to the same > > > > conclusion. I think whatever the outcome is, the boolean should be > > > > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a > > > > help text which mentions "-T /path" and "xenstored --help" to get other > > > > options because there is no man page. > > > > > > > >> Going to a wrapper script just to make some fairly uncommon debugging > > > >> option marginally more convenient seems like overkill to me, plus > > > >> XENSTORED_ARGS would allow for passing other useful options to > > > >> xenstored. > > > > > > > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt > > > > was to expand the XENSTORE variable from the sysconfig file. But this > > > > approach leads to failures with SELinux because the socket passing does > > > > not work this way. Up to now I have not seen a success report for > > > > selinux+systemd+xenstored. Maybe its already somewhere in the other > > > > unread mails. > > > > > > > > In my cover letter I provided some possible ways to handle > > > > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env > > > > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its > > > > possible to select a binary via the sysconfig file. But it also means > > > > the XENSTORE_TRACE boolean has to be removed in favour of the plain > > > > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the > > > > need for a wrapper script. > > > > > > > > Hopefully someone with access to a SELinux enabled system will report > > > > which approach actually works. > > > > > > I can confirm that > > > > > > ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS" > > > > > > does not work on a CentOS7 system with selinux enabled, but that > > > > > > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS > > > > > > > And the difference between these two approaches is /usr/bin/env > > preserves envars while sh -c doesn't? > > /bin/sh doesn't give the right selinux permissions so xenstored doesn't > start. Presumably /usr/bin/env does. > That is "what" but not "why". It could be env is special to selinux, it could be the policy itself is written that way, it could be sh -c throws away something that it shouldn't have. Anyway, I'm just curious. Not going to block this effort just because I don't understand this. I'm pretty sure if this approach is buggy / not universal people who care enough will come back. :-) Wei. > Michael Young ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Thu, 10 Sep 2015, Wei Liu wrote: > On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote: > > On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering wrote: > > > On Tue, Jan 06, Ian Campbell wrote: > > > > > >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote: > > >> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. > > >> > > > >> > Create a separate wrapper script which is used in the sysv runlevel > > >> > script and in systemd xenstored.service. It preserves existing > > >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements > > >> > the handling of XENSTORED_ARGS=. This variable has to be added to > > >> > sysconfig/xencommons. > > >> > > >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an > > >> example in the sysconfig file of enabling tracing if you like)? > > > > > > After having two weeks to think about this I came to the same > > > conclusion. I think whatever the outcome is, the boolean should be > > > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a > > > help text which mentions "-T /path" and "xenstored --help" to get other > > > options because there is no man page. > > > > > >> Going to a wrapper script just to make some fairly uncommon debugging > > >> option marginally more convenient seems like overkill to me, plus > > >> XENSTORED_ARGS would allow for passing other useful options to > > >> xenstored. > > > > > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt > > > was to expand the XENSTORE variable from the sysconfig file. But this > > > approach leads to failures with SELinux because the socket passing does > > > not work this way. Up to now I have not seen a success report for > > > selinux+systemd+xenstored. Maybe its already somewhere in the other > > > unread mails. > > > > > > In my cover letter I provided some possible ways to handle > > > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env > > > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its > > > possible to select a binary via the sysconfig file. But it also means > > > the XENSTORE_TRACE boolean has to be removed in favour of the plain > > > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the > > > need for a wrapper script. > > > > > > Hopefully someone with access to a SELinux enabled system will report > > > which approach actually works. > > > > I can confirm that > > > > ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS" > > > > does not work on a CentOS7 system with selinux enabled, but that > > > > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS > > > > And the difference between these two approaches is /usr/bin/env > preserves envars while sh -c doesn't? /bin/sh doesn't give the right selinux permissions so xenstored doesn't start. Presumably /usr/bin/env does. Michael Young ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote: > On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering wrote: > > On Tue, Jan 06, Ian Campbell wrote: > > > >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote: > >> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. > >> > > >> > Create a separate wrapper script which is used in the sysv runlevel > >> > script and in systemd xenstored.service. It preserves existing > >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements > >> > the handling of XENSTORED_ARGS=. This variable has to be added to > >> > sysconfig/xencommons. > >> > >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an > >> example in the sysconfig file of enabling tracing if you like)? > > > > After having two weeks to think about this I came to the same > > conclusion. I think whatever the outcome is, the boolean should be > > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a > > help text which mentions "-T /path" and "xenstored --help" to get other > > options because there is no man page. > > > >> Going to a wrapper script just to make some fairly uncommon debugging > >> option marginally more convenient seems like overkill to me, plus > >> XENSTORED_ARGS would allow for passing other useful options to > >> xenstored. > > > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt > > was to expand the XENSTORE variable from the sysconfig file. But this > > approach leads to failures with SELinux because the socket passing does > > not work this way. Up to now I have not seen a success report for > > selinux+systemd+xenstored. Maybe its already somewhere in the other > > unread mails. > > > > In my cover letter I provided some possible ways to handle > > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env > > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its > > possible to select a binary via the sysconfig file. But it also means > > the XENSTORE_TRACE boolean has to be removed in favour of the plain > > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the > > need for a wrapper script. > > > > Hopefully someone with access to a SELinux enabled system will report > > which approach actually works. > > I can confirm that > > ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS" > > does not work on a CentOS7 system with selinux enabled, but that > > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS > And the difference between these two approaches is /usr/bin/env preserves envars while sh -c doesn't? Wei. > does work. > > A patch is on its way. > > -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering wrote: > On Tue, Jan 06, Ian Campbell wrote: > >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote: >> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. >> > >> > Create a separate wrapper script which is used in the sysv runlevel >> > script and in systemd xenstored.service. It preserves existing >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements >> > the handling of XENSTORED_ARGS=. This variable has to be added to >> > sysconfig/xencommons. >> >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an >> example in the sysconfig file of enabling tracing if you like)? > > After having two weeks to think about this I came to the same > conclusion. I think whatever the outcome is, the boolean should be > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a > help text which mentions "-T /path" and "xenstored --help" to get other > options because there is no man page. > >> Going to a wrapper script just to make some fairly uncommon debugging >> option marginally more convenient seems like overkill to me, plus >> XENSTORED_ARGS would allow for passing other useful options to >> xenstored. > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt > was to expand the XENSTORE variable from the sysconfig file. But this > approach leads to failures with SELinux because the socket passing does > not work this way. Up to now I have not seen a success report for > selinux+systemd+xenstored. Maybe its already somewhere in the other > unread mails. > > In my cover letter I provided some possible ways to handle > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its > possible to select a binary via the sysconfig file. But it also means > the XENSTORE_TRACE boolean has to be removed in favour of the plain > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the > need for a wrapper script. > > Hopefully someone with access to a SELinux enabled system will report > which approach actually works. I can confirm that ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS" does not work on a CentOS7 system with selinux enabled, but that ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS does work. A patch is on its way. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Wed, Jan 07, 2015 at 03:27:15PM +, Ian Jackson wrote: > Olaf Hering writes ("Re: [PATCH 7/7] tools/hotplug: add wrapper to start > xenstored"): > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt > > was to expand the XENSTORE variable from the sysconfig file. But this > > approach leads to failures with SELinux because the socket passing does > > not work this way. Up to now I have not seen a success report for > > selinux+systemd+xenstored. Maybe its already somewhere in the other > > unread mails. > > The selinux policy should follow the actual code, not vice versa. > > That is, if the approach which we select (based on all the other > criteria) is not compatible with existing selinux policies, this > should be fixed by changing the selinux policies. > > Since the selinux policies are not in xen.git, and are not maintained > as part of the Xen Project, there is no reason to delay introducing > changes in xen.git#master which are known to be incompatible with some > selinux policies. > > My conclusion therefore is that selinux policies are an irrelevant > consideration when deciding what the scripts, systemd integration, > etc. should look like in xen.git#master. > > (And what applies to xen.git#master applies to the as-yet-unreleased > xen.git#staging-4.5 too.) > > > Hopefully someone with access to a SELinux enabled system will report > > which approach actually works. > > I have concluded that the right approach is to disregard selinux. > Developers of selinux-enforcing setups should update the selinux > policies to support what the upstream Xen Project code does. ... which is none. We don't ship any SELinux policies. Anyhow I concur with the sentiment which is why I was aiming at just having an release note about the SELinux part - and having this patch not worry that much about SELinux and instead be satisfactory to you and IanC. Olaf, that hopefully would make it easier for you to come up with a nice patch ? > > Thanks, > Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
Olaf Hering writes ("Re: [PATCH 7/7] tools/hotplug: add wrapper to start xenstored"): > If I recall correctly the point of the current 'sh -c "exec ..."' stunt > was to expand the XENSTORE variable from the sysconfig file. But this > approach leads to failures with SELinux because the socket passing does > not work this way. Up to now I have not seen a success report for > selinux+systemd+xenstored. Maybe its already somewhere in the other > unread mails. The selinux policy should follow the actual code, not vice versa. That is, if the approach which we select (based on all the other criteria) is not compatible with existing selinux policies, this should be fixed by changing the selinux policies. Since the selinux policies are not in xen.git, and are not maintained as part of the Xen Project, there is no reason to delay introducing changes in xen.git#master which are known to be incompatible with some selinux policies. My conclusion therefore is that selinux policies are an irrelevant consideration when deciding what the scripts, systemd integration, etc. should look like in xen.git#master. (And what applies to xen.git#master applies to the as-yet-unreleased xen.git#staging-4.5 too.) > Hopefully someone with access to a SELinux enabled system will report > which approach actually works. I have concluded that the right approach is to disregard selinux. Developers of selinux-enforcing setups should update the selinux policies to support what the upstream Xen Project code does. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Wed, Jan 07, 2015 at 10:49:38AM +0100, Olaf Hering wrote: > On Tue, Jan 06, Ian Jackson wrote: > > > Olaf Hering writes ("[PATCH 7/7] tools/hotplug: add wrapper to start > > xenstored"): > > > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. > > ... > > > +XENSTORED_LIBEXEC = xenstored.sh > > > > Should be in /etc as previously discussed. Previously I wrote: > > > > Bottom line: as relevant maintainer, I'm afraid I'm going to insist > > that this script be in /etc. > > > > I'm disappointed. It is not acceptable to resubmit a change ignoring > > such unequivocal feedback. > > Plain "/etc" wont work, I think. /etc/xen/scripts perhaps? But see my > other reply to IanC, maybe there is a way to avoid the wrapper. > > And after having some time to think about this: If one has a need to > adjust something, then this could be done in the xencommons script right > away. In other words, the modification can be done there instead of > calling the wrapper. > > > Nacked-by: Ian Jackson > > > > > +hotplug/Linux/xenstored.sh > > > > Although many of the existing hotplug scripts have this notion of > > calling things "foo.sh" because they happen to be written in shell, I > > think this is bad practice. > > > > I would prefer xenstored-wrap or some such. (My co-maintainers may > > disagree...) But this is a bit of a bikeshed issue. > > I agree. Initally I had xenstored-launcher in mind. > > > > echo -n Starting $XENSTORED... > > > - $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS > > > + XENSTORED=$XENSTORED \ > > > + XENSTORED_TRACE=$XENSTORED_TRACE \ > > > + XENSTORED_ARGS=$XENSTORED_ARGS \ > > > + ${LIBEXEC_BIN}/xenstored.sh --pid-file > > > /var/run/xenstored.pid > > > > It might be easier to "." xenstore-wrap. Failing that using `export' > > will avoid this rather odd and repetitive style. > > I think thats a good idea. Something like this may work, doing the "." > and the "exec" in the subshell: > > ( > set -- --pid-file /var/run/xenstored.pid > . xenstored.sh > ) > > > > > diff --git a/tools/hotplug/Linux/xenstored.sh.in > > > b/tools/hotplug/Linux/xenstored.sh.in > > > new file mode 100644 > > > index 000..dc806ee > > > --- /dev/null > > > +++ b/tools/hotplug/Linux/xenstored.sh.in > > > @@ -0,0 +1,6 @@ > > > +#!/bin/sh > > > +if test -n "$XENSTORED_TRACE" > > > +then > > > + XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log" > > > +fi > > > +exec $XENSTORED $@ $XENSTORED_ARGS > > > > This should probably have "" around "$@" just in case. > > Ok. > > > I will wait for results from SELinux testing before respinning this > patch. It did work for me (I did an 'Tested-by') in my email. Please do keep in mind that today is the last for commits for Xen 4.5. No pressure :-) ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Tue, Jan 06, Ian Jackson wrote: > Olaf Hering writes ("[PATCH 7/7] tools/hotplug: add wrapper to start > xenstored"): > > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. > ... > > +XENSTORED_LIBEXEC = xenstored.sh > > Should be in /etc as previously discussed. Previously I wrote: > > Bottom line: as relevant maintainer, I'm afraid I'm going to insist > that this script be in /etc. > > I'm disappointed. It is not acceptable to resubmit a change ignoring > such unequivocal feedback. Plain "/etc" wont work, I think. /etc/xen/scripts perhaps? But see my other reply to IanC, maybe there is a way to avoid the wrapper. And after having some time to think about this: If one has a need to adjust something, then this could be done in the xencommons script right away. In other words, the modification can be done there instead of calling the wrapper. > Nacked-by: Ian Jackson > > > +hotplug/Linux/xenstored.sh > > Although many of the existing hotplug scripts have this notion of > calling things "foo.sh" because they happen to be written in shell, I > think this is bad practice. > > I would prefer xenstored-wrap or some such. (My co-maintainers may > disagree...) But this is a bit of a bikeshed issue. I agree. Initally I had xenstored-launcher in mind. > > echo -n Starting $XENSTORED... > > - $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS > > + XENSTORED=$XENSTORED \ > > + XENSTORED_TRACE=$XENSTORED_TRACE \ > > + XENSTORED_ARGS=$XENSTORED_ARGS \ > > + ${LIBEXEC_BIN}/xenstored.sh --pid-file > > /var/run/xenstored.pid > > It might be easier to "." xenstore-wrap. Failing that using `export' > will avoid this rather odd and repetitive style. I think thats a good idea. Something like this may work, doing the "." and the "exec" in the subshell: ( set -- --pid-file /var/run/xenstored.pid . xenstored.sh ) > > diff --git a/tools/hotplug/Linux/xenstored.sh.in > > b/tools/hotplug/Linux/xenstored.sh.in > > new file mode 100644 > > index 000..dc806ee > > --- /dev/null > > +++ b/tools/hotplug/Linux/xenstored.sh.in > > @@ -0,0 +1,6 @@ > > +#!/bin/sh > > +if test -n "$XENSTORED_TRACE" > > +then > > + XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log" > > +fi > > +exec $XENSTORED $@ $XENSTORED_ARGS > > This should probably have "" around "$@" just in case. Ok. I will wait for results from SELinux testing before respinning this patch. Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Tue, Jan 06, Ian Campbell wrote: > On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote: > > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. > > > > Create a separate wrapper script which is used in the sysv runlevel > > script and in systemd xenstored.service. It preserves existing > > behaviour by handling the XENSTORE_TRACE boolean. It also implements > > the handling of XENSTORED_ARGS=. This variable has to be added to > > sysconfig/xencommons. > > Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an > example in the sysconfig file of enabling tracing if you like)? After having two weeks to think about this I came to the same conclusion. I think whatever the outcome is, the boolean should be removed. The sysconfig file should get a XENSTORED_ARGS="" along with a help text which mentions "-T /path" and "xenstored --help" to get other options because there is no man page. > Going to a wrapper script just to make some fairly uncommon debugging > option marginally more convenient seems like overkill to me, plus > XENSTORED_ARGS would allow for passing other useful options to > xenstored. If I recall correctly the point of the current 'sh -c "exec ..."' stunt was to expand the XENSTORE variable from the sysconfig file. But this approach leads to failures with SELinux because the socket passing does not work this way. Up to now I have not seen a success report for selinux+systemd+xenstored. Maybe its already somewhere in the other unread mails. In my cover letter I provided some possible ways to handle selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its possible to select a binary via the sysconfig file. But it also means the XENSTORE_TRACE boolean has to be removed in favour of the plain XENSTORED_ARGS= approach mentioned above. Finally this would avoid the need for a wrapper script. Hopefully someone with access to a SELinux enabled system will report which approach actually works. > > The wrapper uses exec unconditionally. This works because the > > systemd service file passes --no-fork, which has the desired effect > > that the binary launched by systemd becomes the final daemon > > process. The sysv script does not pass --no-fork, which causes > > xenstored to fork internally to return to the caller of the wrapper > > script. > > > > The place of the wrapper is currently LIBEXEC_BIN, it has to be > > decided what the final location is supposed to be. IanJ wants it in > > "/etc". > > If we go this route then I agree with Ian J. (/etc/xen/scripts, I > suppose). I have not heard back which location has to be used. If /etc/xen/scripts is the place, so be it. I thought this is just for hotplug scripts. Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
Olaf Hering writes ("[PATCH 7/7] tools/hotplug: add wrapper to start xenstored"): > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. ... > +XENSTORED_LIBEXEC = xenstored.sh Should be in /etc as previously discussed. Previously I wrote: Bottom line: as relevant maintainer, I'm afraid I'm going to insist that this script be in /etc. I'm disappointed. It is not acceptable to resubmit a change ignoring such unequivocal feedback. Nacked-by: Ian Jackson > +hotplug/Linux/xenstored.sh Although many of the existing hotplug scripts have this notion of calling things "foo.sh" because they happen to be written in shell, I think this is bad practice. I would prefer xenstored-wrap or some such. (My co-maintainers may disagree...) But this is a bit of a bikeshed issue. > echo -n Starting $XENSTORED... > - $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS > + XENSTORED=$XENSTORED \ > + XENSTORED_TRACE=$XENSTORED_TRACE \ > + XENSTORED_ARGS=$XENSTORED_ARGS \ > + ${LIBEXEC_BIN}/xenstored.sh --pid-file > /var/run/xenstored.pid It might be easier to "." xenstore-wrap. Failing that using `export' will avoid this rather odd and repetitive style. > diff --git a/tools/hotplug/Linux/xenstored.sh.in > b/tools/hotplug/Linux/xenstored.sh.in > new file mode 100644 > index 000..dc806ee > --- /dev/null > +++ b/tools/hotplug/Linux/xenstored.sh.in > @@ -0,0 +1,6 @@ > +#!/bin/sh > +if test -n "$XENSTORED_TRACE" > +then > + XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log" > +fi > +exec $XENSTORED $@ $XENSTORED_ARGS This should probably have "" around "$@" just in case. Thanks, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored
On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote: > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE. > > Create a separate wrapper script which is used in the sysv runlevel > script and in systemd xenstored.service. It preserves existing > behaviour by handling the XENSTORE_TRACE boolean. It also implements > the handling of XENSTORED_ARGS=. This variable has to be added to > sysconfig/xencommons. Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an example in the sysconfig file of enabling tracing if you like)? Going to a wrapper script just to make some fairly uncommon debugging option marginally more convenient seems like overkill to me, plus XENSTORED_ARGS would allow for passing other useful options to xenstored. > The wrapper uses exec unconditionally. This works because the > systemd service file passes --no-fork, which has the desired effect > that the binary launched by systemd becomes the final daemon > process. The sysv script does not pass --no-fork, which causes > xenstored to fork internally to return to the caller of the wrapper > script. > > The place of the wrapper is currently LIBEXEC_BIN, it has to be > decided what the final location is supposed to be. IanJ wants it in > "/etc". If we go this route then I agree with Ian J. (/etc/xen/scripts, I suppose). Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel