Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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 <olaf@xxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel