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

[Olaf Hering]

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]

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

[Wei Liu]

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

[George Dunlap]

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

[George Dunlap]

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

[M A Young]

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

[Ian Jackson]

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

[Olaf Hering]

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 ian.jack...@eu.citrix.com
 
  +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

[Olaf Hering]

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

[Konrad Rzeszutek Wilk]

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 ian.jack...@eu.citrix.com
  
   +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

[Ian Jackson]

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

[Konrad Rzeszutek Wilk]

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

[Ian Campbell]

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

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

[Ian Jackson]

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 ian.jack...@eu.citrix.com

 +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

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

[Olaf Hering]

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.

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.

Signed-off-by: Olaf Hering o...@aepfle.de
Cc: Ian Jackson ian.jack...@eu.citrix.com
Cc: Stefano Stabellini stefano.stabell...@eu.citrix.com
Cc: Ian Campbell ian.campb...@citrix.com
Cc: Wei Liu wei.l...@citrix.com
---
 .gitignore   | 1 +
 tools/configure  | 3 ++-
 tools/configure.ac   | 1 +
 tools/hotplug/Linux/Makefile | 2 ++
 tools/hotplug/Linux/init.d/xencommons.in | 6 --
 tools/hotplug/Linux/systemd/xenstored.service.in | 5 ++---
 tools/hotplug/Linux/xenstored.sh.in  | 6 ++
 7 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/.gitignore b/.gitignore
index 8c8c06f..7e6884a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -153,6 +153,7 @@ tools/hotplug/Linux/vif-setup
 tools/hotplug/Linux/xen-backend.rules
 tools/hotplug/Linux/xen-hotplug-common.sh
 tools/hotplug/Linux/xendomains
+tools/hotplug/Linux/xenstored.sh
 tools/hotplug/NetBSD/rc.d/xencommons
 tools/include/xen/*
 tools/include/xen-foreign/*.(c|h|size)
diff --git a/tools/configure b/tools/configure
index b0aea0a..e72876c 100755
--- a/tools/configure
+++ b/tools/configure
@@ -2276,7 +2276,7 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
-ac_config_files=$ac_config_files ../config/Tools.mk 
hotplug/FreeBSD/rc.d/xencommons hotplug/Linux/init.d/sysconfig.xencommons 
hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons 
hotplug/Linux/init.d/xendomains hotplug/Linux/systemd/proc-xen.mount 
hotplug/Linux/systemd/var-lib-xenstored.mount 
hotplug/Linux/systemd/xen-init-dom0.service 
hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service 
hotplug/Linux/systemd/xen-watchdog.service 
hotplug/Linux/systemd/xenconsoled.service 
hotplug/Linux/systemd/xendomains.service 
hotplug/Linux/systemd/xenstored.service hotplug/Linux/systemd/xenstored.socket 
hotplug/Linux/systemd/xenstored_ro.socket hotplug/Linux/vif-setup 
hotplug/Linux/xen-backend.rules hotplug/Linux/xen-hotplug-common.sh 
hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons
+ac_config_files=$ac_config_files ../config/Tools.mk 
hotplug/FreeBSD/rc.d/xencommons hotplug/Linux/init.d/sysconfig.xencommons 
hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons 
hotplug/Linux/init.d/xendomains hotplug/Linux/systemd/proc-xen.mount 
hotplug/Linux/systemd/var-lib-xenstored.mount 
hotplug/Linux/systemd/xen-init-dom0.service 
hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service 
hotplug/Linux/systemd/xen-watchdog.service 
hotplug/Linux/systemd/xenconsoled.service 
hotplug/Linux/systemd/xendomains.service 
hotplug/Linux/systemd/xenstored.service hotplug/Linux/systemd/xenstored.socket 
hotplug/Linux/systemd/xenstored_ro.socket hotplug/Linux/vif-setup 
hotplug/Linux/xen-backend.rules hotplug/Linux/xen-hotplug-common.sh 
hotplug/Linux/xendomains hotplug/Linux/xenstored.sh 
hotplug/NetBSD/rc.d/xencommons
 
 ac_config_headers=$ac_config_headers config.h
 
@@ -9585,6 +9585,7 @@ do
 hotplug/Linux/xen-backend.rules) CONFIG_FILES=$CONFIG_FILES 
hotplug/Linux/xen-backend.rules ;;
 hotplug/Linux/xen-hotplug-common.sh) CONFIG_FILES=$CONFIG_FILES 
hotplug/Linux/xen-hotplug-common.sh ;;
 hotplug/Linux/xendomains) CONFIG_FILES=$CONFIG_FILES 
hotplug/Linux/xendomains ;;
+hotplug/Linux/xenstored.sh) CONFIG_FILES=$CONFIG_FILES 
hotplug/Linux/xenstored.sh ;;
 hotplug/NetBSD/rc.d/xencommons) CONFIG_FILES=$CONFIG_FILES 
hotplug/NetBSD/rc.d/xencommons ;;
 config.h) CONFIG_HEADERS=$CONFIG_HEADERS config.h ;;
 
diff --git a/tools/configure.ac b/tools/configure.ac
index 1ac63a3..8f198e8 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -26,6 +26,7 @@ hotplug/Linux/vif-setup
 hotplug/Linux/xen-backend.rules
 hotplug/Linux/xen-hotplug-common.sh
 hotplug/Linux/xendomains
+hotplug/Linux/xenstored.sh
 hotplug/NetBSD/rc.d/xencommons
 ])
 AC_CONFIG_HEADERS([config.h])
diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 1706c05..e9a1ef0 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -2,6 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include