[PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

This is a resend of these two series:
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00858.html
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html

New in v3 is a wrapper to run xenstored. See its patch description
for details.

Patch 2-6 should be applied for 4.5.0.

The first and the last one still has issues with xenstored and
SELinux. See below.  Up to now no solution is known to me.


The first patch fixes Arch Linux and does not break anything.  As such
it should be safe to be applied for 4.5.0.  SELinux users (who build
from source) should put their special mount options into fstab. Distro
packages will most likely include a proper .service file.


The last patch addresses the XENSTORED_TRACE issue. But SELinux will
most likely still not work.

Possible ways to handle launching xenstored and SELinux:

- do nothing
  pro: - no Xen source changes required
  con: - possible unhappy users who build from source and still have
         SELinux enabled

- use newly added wrapper
  pro: - XENSTORED_TRACE boolean is handled
  con: - the wrapper may have the very same issue as the current
         launching with sh -c 'exec xenstored'. But maybe there is a
	 way to mark the new wrapper script as "this is the native
	 xenstored". Someone familiar with SELinux may be able to
	 answer this.

- Use ExecStart=@XENSTORED@
  pro: - socket passing will most likely work
  con: - All options have to be passed in XENSTORED_ARGS, a new variable
         which is not yet mentioned in the sysconfig file.
       - Switching xenstored requires a private copy of
	 xenstored.service in /etc/systemd instead of adjusting the
	 XENSTORED= variable in the sysconfig file.

- Use ExecStart=/usr/bin/env $XENSTORED
  pro: - $XENSTORED can be set in sysconfig file
  con: - may have the same socket issue as starting via shell
       - XENSTORED_TRACE boolean is not handled


I will be offline until 2015-01-07, so any further adjustments to this
series has to be done by someone else.


Good luck!

Olaf


Olaf Hering (7):
  tools/hotplug: remove SELinux options from var-lib-xenstored.mount
  tools/hotplug: remove XENSTORED_ROOTDIR from xenstored.service
  tools/hotplug: xendomains.service depends on network
  tools/hotplug: use xencommons as EnvironmentFile in
    xenconsoled.service
  tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service
  tools/hotplug: remove EnvironmentFile from
    xen-qemu-dom0-disk-backend.service
  tools/hotplug: add wrapper to start xenstored

 .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/var-lib-xenstored.mount.in            | 4 +---
 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 -
 tools/hotplug/Linux/systemd/xenconsoled.service.in                | 6 +++---
 tools/hotplug/Linux/systemd/xendomains.service.in                 | 2 ++
 tools/hotplug/Linux/systemd/xenstored.service.in                  | 6 ++----
 tools/hotplug/Linux/xenstored.sh.in                               | 6 ++++++
 11 files changed, 24 insertions(+), 14 deletions(-)
 create mode 100644 tools/hotplug/Linux/xenstored.sh.in

[PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Olaf Hering]

Using SELinux mount options per default breaks several systems.
Either the context= mount option is not known at all to the kernel,
as reported for ArchLinux. Or the default value "none" is unknown to
SELinux, as reported for Fedora. In both cases the unit will fail.

The proper place to specify mount options is /etc/fstab. Appearently
systemd is kind enough to use values from there even if Options= or
What= is specified in a .mount file.

Remove XENSTORED_MOUNT_CTX, the reference to a non-existant
EnvironmentFile and trim default Options= for the mount point.

The removed code was first mentioned in the patch referenced below,
with the following description:
...
 * Some systems define the selinux context in the systemd Option for
   the /var/lib/xenstored tmpfs:
     Options=mode=755,context="system_u:object_r:xenstored_var_lib_t:s0"
   For the upstream version we remove that and let systems specify
   the context on their system /etc/default/xenstored or
   /etc/sysconfig/xenstored $XENSTORED_MOUNT_CTX variable
...
It is nowhere stated (on xen-devel) what "Some systems" means, which
is unfortunately common practice in nearly all opensource projects.
http://lists.xenproject.org/archives/html/xen-devel/2014-03/msg02462.html

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: M A Young <m.a.young@durham.ac.uk>
Cc: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
index d5e04db..11a7d50 100644
--- a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
+++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
@@ -6,9 +6,7 @@ ConditionPathExists=/proc/xen/capabilities
 RefuseManualStop=true
 
 [Mount]
-Environment=XENSTORED_MOUNT_CTX=none
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
 What=xenstore
 Where=@XEN_LIB_STORED@
 Type=tmpfs
-Options=mode=755,context="$XENSTORED_MOUNT_CTX"
+Options=mode=755

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Ian Campbell]

On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> Using SELinux mount options per default breaks several systems.
> Either the context= mount option is not known at all to the kernel,
> as reported for ArchLinux. Or the default value "none" is unknown to
> SELinux, as reported for Fedora. In both cases the unit will fail.
> 
> The proper place to specify mount options is /etc/fstab. Appearently
> systemd is kind enough to use values from there even if Options= or
> What= is specified in a .mount file.
> 
> Remove XENSTORED_MOUNT_CTX, the reference to a non-existant
> EnvironmentFile and trim default Options= for the mount point.
> 
> The removed code was first mentioned in the patch referenced below,
> with the following description:
> ...
>  * Some systems define the selinux context in the systemd Option for
>    the /var/lib/xenstored tmpfs:
>      Options=mode=755,context="system_u:object_r:xenstored_var_lib_t:s0"
>    For the upstream version we remove that and let systems specify
>    the context on their system /etc/default/xenstored or
>    /etc/sysconfig/xenstored $XENSTORED_MOUNT_CTX variable
> ...
> It is nowhere stated (on xen-devel) what "Some systems" means, which
> is unfortunately common practice in nearly all opensource projects.
> http://lists.xenproject.org/archives/html/xen-devel/2014-03/msg02462.html
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(on commit s/Appearently/Apparently/; s/non-existant/non-existent/ in
the commit log)

> -Options=mode=755,context="$XENSTORED_MOUNT_CTX"
> +Options=mode=755

FWIW an alternative might have been:
  Options=mode=755,$XENSTORED_MOUNT_OPTIONS
where the variable from the EnvironmentFile could contain context= as
necessary (and maybe even mode=... by default).

But if /etc/fstab is the Right Place(tm) then lets go with that for 4.5.

Ian.

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Olaf Hering]

On Tue, Jan 06, Ian Campbell wrote:

> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:

...

> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> (on commit s/Appearently/Apparently/; s/non-existant/non-existent/ in
> the commit log)

I made typos also in other commit messages. Should I resend the entire
series, or will this be done during commit?

Olaf

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Ian Campbell]

On Wed, 2015-01-07 at 10:23 +0100, Olaf Hering wrote:
> On Tue, Jan 06, Ian Campbell wrote:
> 
> > On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> 
> ...
> 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > (on commit s/Appearently/Apparently/; s/non-existant/non-existent/ in
> > the commit log)
> 
> I made typos also in other commit messages. Should I resend the entire
> series, or will this be done during commit?

Looks like Konrad already committed, I don't know if he fixed the typos
(I suppose it doesn't matter now either way).

Ian.

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Konrad Rzeszutek Wilk]

On Wed, Jan 07, 2015 at 09:31:50AM +0000, Ian Campbell wrote:
> On Wed, 2015-01-07 at 10:23 +0100, Olaf Hering wrote:
> > On Tue, Jan 06, Ian Campbell wrote:
> > 
> > > On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> > 
> > ...
> > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > (on commit s/Appearently/Apparently/; s/non-existant/non-existent/ in
> > > the commit log)
> > 
> > I made typos also in other commit messages. Should I resend the entire
> > series, or will this be done during commit?
> 
> Looks like Konrad already committed, I don't know if he fixed the typos
> (I suppose it doesn't matter now either way).

I did the changes you pointed our here.
> 
> Ian.
> 
> 

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Ian Jackson]

Olaf Hering writes ("[PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount"):
> Using SELinux mount options per default breaks several systems.
> Either the context= mount option is not known at all to the kernel,
> as reported for ArchLinux. Or the default value "none" is unknown to
> SELinux, as reported for Fedora. In both cases the unit will fail.
...
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

When applied along with the README change provided by Konrad.

Ian.

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On Fri, Dec 19, 2014 at 11:25 AM, Olaf Hering <olaf@aepfle.de> wrote:
> Using SELinux mount options per default breaks several systems.
> Either the context= mount option is not known at all to the kernel,
> as reported for ArchLinux. Or the default value "none" is unknown to
> SELinux, as reported for Fedora. In both cases the unit will fail.
>
> The proper place to specify mount options is /etc/fstab. Appearently
> systemd is kind enough to use values from there even if Options= or
> What= is specified in a .mount file.

For the benefit of someone moonlighting as a CentOS package
maintainer, could you tell me how adding such an entry in a package is
normally done?  Or alternately, how you would recommend a package
maintainer to add the appropriate context?

 -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[M A Young]

On Thu, 10 Sep 2015, George Dunlap wrote:

> On Fri, Dec 19, 2014 at 11:25 AM, Olaf Hering <olaf@aepfle.de> wrote:
> > Using SELinux mount options per default breaks several systems.
> > Either the context= mount option is not known at all to the kernel,
> > as reported for ArchLinux. Or the default value "none" is unknown to
> > SELinux, as reported for Fedora. In both cases the unit will fail.
> >
> > The proper place to specify mount options is /etc/fstab. Appearently
> > systemd is kind enough to use values from there even if Options= or
> > What= is specified in a .mount file.
> 
> For the benefit of someone moonlighting as a CentOS package
> maintainer, could you tell me how adding such an entry in a package is
> normally done?  Or alternately, how you would recommend a package
> maintainer to add the appropriate context?

I suspect it is actually easier to put the selinux context back into 
systemd file rather than trying to edit /etc/fstab which could get messy.
If that is what you want to do you could look at 
http://pkgs.fedoraproject.org/cgit/xen.git/tree/xen.fedora.systemd.patch
for ideas on how to do it.

	Michael Young

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On 09/10/2015 03:13 PM, M A Young wrote:
> On Thu, 10 Sep 2015, George Dunlap wrote:
> 
>> On Fri, Dec 19, 2014 at 11:25 AM, Olaf Hering <olaf@aepfle.de> wrote:
>>> Using SELinux mount options per default breaks several systems.
>>> Either the context= mount option is not known at all to the kernel,
>>> as reported for ArchLinux. Or the default value "none" is unknown to
>>> SELinux, as reported for Fedora. In both cases the unit will fail.
>>>
>>> The proper place to specify mount options is /etc/fstab. Appearently
>>> systemd is kind enough to use values from there even if Options= or
>>> What= is specified in a .mount file.
>>
>> For the benefit of someone moonlighting as a CentOS package
>> maintainer, could you tell me how adding such an entry in a package is
>> normally done?  Or alternately, how you would recommend a package
>> maintainer to add the appropriate context?
> 
> I suspect it is actually easier to put the selinux context back into 
> systemd file rather than trying to edit /etc/fstab which could get messy.
> If that is what you want to do you could look at 
> http://pkgs.fedoraproject.org/cgit/xen.git/tree/xen.fedora.systemd.patch
> for ideas on how to do it.

Right, well manually modifying the upstream source file is not a good
"interface" to provide.  If modifying /etc/fstab is not "the right
solution" for packages, then much better solution would have been to do
what IanC suggested later in this thread, and do something like this
instead:

Options=mode=755,$XENSTORED_MOUNT_OPTIONS

 -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Olaf Hering]

On Thu, Sep 10, George Dunlap wrote:

> On Fri, Dec 19, 2014 at 11:25 AM, Olaf Hering <olaf@aepfle.de> wrote:
> > Using SELinux mount options per default breaks several systems.
> > Either the context= mount option is not known at all to the kernel,
> > as reported for ArchLinux. Or the default value "none" is unknown to
> > SELinux, as reported for Fedora. In both cases the unit will fail.
> >
> > The proper place to specify mount options is /etc/fstab. Appearently
> > systemd is kind enough to use values from there even if Options= or
> > What= is specified in a .mount file.
> 
> For the benefit of someone moonlighting as a CentOS package
> maintainer, could you tell me how adding such an entry in a package is
> normally done?  Or alternately, how you would recommend a package
> maintainer to add the appropriate context?

George, I know nothing about SELinux.
I think its either up to a rpm %post install script to fiddle with fstab
and pray that the added lines fit the system policies. Or its up to the
documentation team to describe how SELinux is supposed to be configured
for the third party app "Xen" on CentOS.

Olaf

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On 09/11/2015 07:31 AM, Olaf Hering wrote:
> On Thu, Sep 10, George Dunlap wrote:
> 
>> On Fri, Dec 19, 2014 at 11:25 AM, Olaf Hering <olaf@aepfle.de> wrote:
>>> Using SELinux mount options per default breaks several systems.
>>> Either the context= mount option is not known at all to the kernel,
>>> as reported for ArchLinux. Or the default value "none" is unknown to
>>> SELinux, as reported for Fedora. In both cases the unit will fail.
>>>
>>> The proper place to specify mount options is /etc/fstab. Appearently
>>> systemd is kind enough to use values from there even if Options= or
>>> What= is specified in a .mount file.
>>
>> For the benefit of someone moonlighting as a CentOS package
>> maintainer, could you tell me how adding such an entry in a package is
>> normally done?  Or alternately, how you would recommend a package
>> maintainer to add the appropriate context?
> 
> George, I know nothing about SELinux.
> I think its either up to a rpm %post install script to fiddle with fstab
> and pray that the added lines fit the system policies. Or its up to the
> documentation team to describe how SELinux is supposed to be configured
> for the third party app "Xen" on CentOS.

Well if you "know nothing about SELinux", and you don't use it, and
don't have any test systems that use it, then why did you assert
"The proper place to specify [an SELinux mount context] is /etc/fstab"?
 This patchset was accepted because you represented it as the "right"
way of doing things.

So poking around CentOS 7, it looks like in most cases, after a tmpfs is
mounted, "restorecon -R $mountpoint" is also run, which restores the
default selinux tags.  Manually starting var-lib-xenstored, then running
restorecon, then manually starting xenstored.service seems to work.  So
at the moment I'm trying to figure out if there's a "right" way to get
restorecon run at the right time  (or alternately, a "right" way to
mount a tmpfs at /var/lib/xenstored such that it happens automatically).

If that doesn't work, then adding a xenstored configuration file that
can contain mount options is probably the best option.

 -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Olaf Hering]

On Mon, Sep 14, George Dunlap wrote:

> Well if you "know nothing about SELinux", and you don't use it, and
> don't have any test systems that use it, then why did you assert
> "The proper place to specify [an SELinux mount context] is /etc/fstab"?
>  This patchset was accepted because you represented it as the "right"
> way of doing things.

Because at that time the way SELinux was handled failed on systems which
had SELinux disabled, or which did not recognize the option.
And I still think that mount options have to go into fstab.

Olaf

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On Mon, Sep 14, 2015 at 7:33 PM, Olaf Hering <olaf@aepfle.de> wrote:
> On Mon, Sep 14, George Dunlap wrote:
>
>> Well if you "know nothing about SELinux", and you don't use it, and
>> don't have any test systems that use it, then why did you assert
>> "The proper place to specify [an SELinux mount context] is /etc/fstab"?
>>  This patchset was accepted because you represented it as the "right"
>> way of doing things.
>
> Because at that time the way SELinux was handled failed on systems which
> had SELinux disabled, or which did not recognize the option.
> And I still think that mount options have to go into fstab.

It's very reasonable for you to expect it to be fixed on non-SELinux
systems.  But what you did is fix it for non-SELinux systems by simply
breaking it on SELinux systems -- that's not at all reasonable.

And I'm not really familiar enough with the standards around fstab and
whatever to have a strong opinion on the "right" way to do things; but
"fiddle with fstab and pray that the added lines fit the system
policies" is definitely not my idea of the Right Way to do things.

In any case, it looks like adding manual mount options isn't actually
the Right Way to do fix things for SELinux, no matter where you put
them -- it requires your mount options to be kept in sync with the
global SELinux policy, which is more fragile.  The way most other
tmpfs things get dealt with, as I said, is running "restorecon", which
updates labels from the master SELinux policy.

 -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Olaf Hering]

On Tue, Sep 15, George Dunlap wrote:

> It's very reasonable for you to expect it to be fixed on non-SELinux
> systems.  But what you did is fix it for non-SELinux systems by simply
> breaking it on SELinux systems -- that's not at all reasonable.

Konrad did some testing at that time and said 4.5 was ok.
Why is 4.6 broken now?

Olaf

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On Tue, Sep 15, 2015 at 1:48 PM, Olaf Hering <olaf@aepfle.de> wrote:
> On Tue, Sep 15, George Dunlap wrote:
>
>> It's very reasonable for you to expect it to be fixed on non-SELinux
>> systems.  But what you did is fix it for non-SELinux systems by simply
>> breaking it on SELinux systems -- that's not at all reasonable.
>
> Konrad did some testing at that time and said 4.5 was ok.
> Why is 4.6 broken now?

OK -- I see that he committed it, but I didn't see him say that he had
tested this particular patch.  It would be interesting to find out why
it worked for him.

 -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Konrad Rzeszutek Wilk]

On Tue, Sep 15, 2015 at 01:55:15PM +0100, George Dunlap wrote:
> On Tue, Sep 15, 2015 at 1:48 PM, Olaf Hering <olaf@aepfle.de> wrote:
> > On Tue, Sep 15, George Dunlap wrote:
> >
> >> It's very reasonable for you to expect it to be fixed on non-SELinux
> >> systems.  But what you did is fix it for non-SELinux systems by simply
> >> breaking it on SELinux systems -- that's not at all reasonable.
> >
> > Konrad did some testing at that time and said 4.5 was ok.
> > Why is 4.6 broken now?
> 
> OK -- I see that he committed it, but I didn't see him say that he had
> tested this particular patch.  It would be interesting to find out why
> it worked for him.

It just worked out of the box when I installed an source build of the Xen
on a virgin Fedora box.

I am not sure how it worked if SELinux ended up being disabled!

> 
>  -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On 09/15/2015 02:58 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 15, 2015 at 01:55:15PM +0100, George Dunlap wrote:
>> On Tue, Sep 15, 2015 at 1:48 PM, Olaf Hering <olaf@aepfle.de> wrote:
>>> On Tue, Sep 15, George Dunlap wrote:
>>>
>>>> It's very reasonable for you to expect it to be fixed on non-SELinux
>>>> systems.  But what you did is fix it for non-SELinux systems by simply
>>>> breaking it on SELinux systems -- that's not at all reasonable.
>>>
>>> Konrad did some testing at that time and said 4.5 was ok.
>>> Why is 4.6 broken now?
>>
>> OK -- I see that he committed it, but I didn't see him say that he had
>> tested this particular patch.  It would be interesting to find out why
>> it worked for him.
> 
> It just worked out of the box when I installed an source build of the Xen
> on a virgin Fedora box.
> 
> I am not sure how it worked if SELinux ended up being disabled!

So how did you install Xen?  "make install"?  Or did you do "make rpmball"?

Is it possible that /usr/sbin/xenstored never got the default selinux
label, and so never had any issues from the fact that /var/lib/xenstored
also didn't have the proper label?

 -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Konrad Rzeszutek Wilk]

On Tue, Sep 15, 2015 at 03:01:31PM +0100, George Dunlap wrote:
> On 09/15/2015 02:58 PM, Konrad Rzeszutek Wilk wrote:
> > On Tue, Sep 15, 2015 at 01:55:15PM +0100, George Dunlap wrote:
> >> On Tue, Sep 15, 2015 at 1:48 PM, Olaf Hering <olaf@aepfle.de> wrote:
> >>> On Tue, Sep 15, George Dunlap wrote:
> >>>
> >>>> It's very reasonable for you to expect it to be fixed on non-SELinux
> >>>> systems.  But what you did is fix it for non-SELinux systems by simply
> >>>> breaking it on SELinux systems -- that's not at all reasonable.
> >>>
> >>> Konrad did some testing at that time and said 4.5 was ok.
> >>> Why is 4.6 broken now?
> >>
> >> OK -- I see that he committed it, but I didn't see him say that he had
> >> tested this particular patch.  It would be interesting to find out why
> >> it worked for him.
> > 
> > It just worked out of the box when I installed an source build of the Xen
> > on a virgin Fedora box.
> > 
> > I am not sure how it worked if SELinux ended up being disabled!
> 
> So how did you install Xen?  "make install"?  Or did you do "make rpmball"?

./configure --enable-systemd --prefix=/usr 

make -j31556
make install

cat README | grep systemctl
[paste all of those in the command line]

grub2-mkconfig -o /boot/grub/grub2.cfg

reboot

> 
> Is it possible that /usr/sbin/xenstored never got the default selinux
> label, and so never had any issues from the fact that /var/lib/xenstored
> also didn't have the proper label?


I think you are asking me to try this once more and seeing if
I see the error you think I should be seeing :-)

I can certainly do that - but not today. Would Friday be OK?

> 
>  -George
> 

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On 09/15/2015 04:12 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 15, 2015 at 03:01:31PM +0100, George Dunlap wrote:
>> On 09/15/2015 02:58 PM, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Sep 15, 2015 at 01:55:15PM +0100, George Dunlap wrote:
>>>> On Tue, Sep 15, 2015 at 1:48 PM, Olaf Hering <olaf@aepfle.de> wrote:
>>>>> On Tue, Sep 15, George Dunlap wrote:
>>>>>
>>>>>> It's very reasonable for you to expect it to be fixed on non-SELinux
>>>>>> systems.  But what you did is fix it for non-SELinux systems by simply
>>>>>> breaking it on SELinux systems -- that's not at all reasonable.
>>>>>
>>>>> Konrad did some testing at that time and said 4.5 was ok.
>>>>> Why is 4.6 broken now?
>>>>
>>>> OK -- I see that he committed it, but I didn't see him say that he had
>>>> tested this particular patch.  It would be interesting to find out why
>>>> it worked for him.
>>>
>>> It just worked out of the box when I installed an source build of the Xen
>>> on a virgin Fedora box.
>>>
>>> I am not sure how it worked if SELinux ended up being disabled!
>>
>> So how did you install Xen?  "make install"?  Or did you do "make rpmball"?
> 
> ./configure --enable-systemd --prefix=/usr 
> 
> make -j31556
> make install
> 
> cat README | grep systemctl
> [paste all of those in the command line]
> 
> grub2-mkconfig -o /boot/grub/grub2.cfg
> 
> reboot

Right -- so you never did "restorecon" or "fixfiles -f relabel" or
"touch /.autorelabel" or anything explicitly to give the installed
binares their selinux labels?

In which case I'm *guessing* that you never actually set up selinux for
the Xen binares, and the reason it worked for you was that you weren't
actualling using the selinux rules.

>> Is it possible that /usr/sbin/xenstored never got the default selinux
>> label, and so never had any issues from the fact that /var/lib/xenstored
>> also didn't have the proper label?
> 
> 
> I think you are asking me to try this once more and seeing if
> I see the error you think I should be seeing :-)
> 
> I can certainly do that - but not today. Would Friday be OK?

Well, I did think about asking you to try again, but I purposely didn't.
:-)

Since you've offered though, yes, it would be good if you could do
exactly what you did before, and then look at

ls -lZ /usr/sbin/xenstored

And then, perhaps, do "touch /.autorelabel" (assuming that works on
Fedora the way it works on CentOS), reboot, and see what happens (and
what ls -lZ /usr/sbin/xenstored comes up with)?

I won't be working Friday, but I'll be back in Monday.

Thanks,
 -George

Re: [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Konrad Rzeszutek Wilk]

On Tue, Sep 15, 2015 at 02:48:57PM +0200, Olaf Hering wrote:
> On Tue, Sep 15, George Dunlap wrote:
> 
> > It's very reasonable for you to expect it to be fixed on non-SELinux
> > systems.  But what you did is fix it for non-SELinux systems by simply
> > breaking it on SELinux systems -- that's not at all reasonable.
> 
> Konrad did some testing at that time and said 4.5 was ok.

Correct.
> Why is 4.6 broken now?
> 
> Olaf
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH 2/7] tools/hotplug: remove XENSTORED_ROOTDIR from xenstored.service

[Olaf Hering]

There is no need to export XENSTORED_ROOTDIR. This variable can be
enabled in sysconfig/xencommons. If the variable is unset xenstored
will automatically use @XEN_LIB_STORED@.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xenstored.service.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 780bdd6..0f0ac58 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -9,7 +9,6 @@ ConditionPathExists=/proc/xen/capabilities
 [Service]
 Type=notify
 Environment=XENSTORED_ARGS=
-Environment=XENSTORED_ROOTDIR=@XEN_LIB_STORED@
 Environment=XENSTORED=@XENSTORED@
 EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities

[PATCH 3/7] tools/hotplug: xendomains.service depends on network

[Olaf Hering]

Starting domains during boot will most likely require network for
the local bridge and it may need access to remote filesystems. Add
ordering tags to systemd service file.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xendomains.service.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/hotplug/Linux/systemd/xendomains.service.in b/tools/hotplug/Linux/systemd/xendomains.service.in
index 9962671..66e2065 100644
--- a/tools/hotplug/Linux/systemd/xendomains.service.in
+++ b/tools/hotplug/Linux/systemd/xendomains.service.in
@@ -2,6 +2,8 @@
 Description=Xendomains - start and stop guests on boot and shutdown
 Requires=proc-xen.mount xenstored.service
 After=proc-xen.mount xenstored.service xenconsoled.service xen-init-dom0.service
+After=network-online.target
+After=remote-fs.target
 ConditionPathExists=/proc/xen/capabilities
 
 [Service]

[PATCH 4/7] tools/hotplug: use xencommons as EnvironmentFile in xenconsoled.service

[Olaf Hering]

The referenced sysconfig/xenconsoled does not exist. If anything
needs to be specified it has to go into the existing
sysconfig/xencommons file.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xenconsoled.service.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
index cb44cd6..74d0428 100644
--- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
+++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
@@ -9,7 +9,7 @@ Type=simple
 Environment=XENCONSOLED_ARGS=
 Environment=XENCONSOLED_LOG=none
 Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenconsoled
+EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}

Re: [PATCH 4/7] tools/hotplug: use xencommons as EnvironmentFile in xenconsoled.service

[Ian Campbell]

On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> The referenced sysconfig/xenconsoled does not exist. If anything
> needs to be specified it has to go into the existing
> sysconfig/xencommons file.

That seems consistent with the initscript and the xencommons sysconfig
template even includes XENCONSOLED_TRACE as an example, so I think this
is correct.

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/hotplug/Linux/systemd/xenconsoled.service.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
> index cb44cd6..74d0428 100644
> --- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
> +++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
> @@ -9,7 +9,7 @@ Type=simple
>  Environment=XENCONSOLED_ARGS=
>  Environment=XENCONSOLED_LOG=none
>  Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
> -EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenconsoled
> +EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
>  PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
>  ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>  ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}

Re: [PATCH 4/7] tools/hotplug: use xencommons as EnvironmentFile in xenconsoled.service

[Ian Jackson]

Olaf Hering writes ("[PATCH 4/7] tools/hotplug: use xencommons as EnvironmentFile in xenconsoled.service"):
> The referenced sysconfig/xenconsoled does not exist. If anything
> needs to be specified it has to go into the existing
> sysconfig/xencommons file.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

[PATCH 5/7] tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service

[Olaf Hering]

Instead of inventing a new XENCONSOLED_LOG= variable reuse the
existing XENCONSOLED_TRACE= variable in xenconsoled.service.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xenconsoled.service.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
index 74d0428..4788129 100644
--- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
+++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
@@ -7,13 +7,13 @@ ConditionPathExists=/proc/xen/capabilities
 [Service]
 Type=simple
 Environment=XENCONSOLED_ARGS=
-Environment=XENCONSOLED_LOG=none
+Environment=XENCONSOLED_TRACE=none
 Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
 EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}
-ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_LOG} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
+ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_TRACE} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
 
 [Install]
 WantedBy=multi-user.target

Re: [PATCH 5/7] tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service

[Ian Campbell]

On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> Instead of inventing a new XENCONSOLED_LOG= variable reuse the
> existing XENCONSOLED_TRACE= variable in xenconsoled.service.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

(should XENCONSOLED_LOG_DIR be changed for consistency?)

> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/hotplug/Linux/systemd/xenconsoled.service.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
> index 74d0428..4788129 100644
> --- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
> +++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
> @@ -7,13 +7,13 @@ ConditionPathExists=/proc/xen/capabilities
>  [Service]
>  Type=simple
>  Environment=XENCONSOLED_ARGS=
> -Environment=XENCONSOLED_LOG=none
> +Environment=XENCONSOLED_TRACE=none
>  Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
>  EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
>  PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
>  ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>  ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}
> -ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_LOG} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
> +ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_TRACE} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
>  
>  [Install]
>  WantedBy=multi-user.target

Re: [PATCH 5/7] tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service

[Konrad Rzeszutek Wilk]

On Tue, Jan 06, 2015 at 11:30:38AM +0000, Ian Campbell wrote:
> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> > Instead of inventing a new XENCONSOLED_LOG= variable reuse the
> > existing XENCONSOLED_TRACE= variable in xenconsoled.service.
> > 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> (should XENCONSOLED_LOG_DIR be changed for consistency?)

To XENCONSOLED_TRACE_DIR ? It could but lets leave that for
another patch.

> 
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/hotplug/Linux/systemd/xenconsoled.service.in | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
> > index 74d0428..4788129 100644
> > --- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
> > +++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
> > @@ -7,13 +7,13 @@ ConditionPathExists=/proc/xen/capabilities
> >  [Service]
> >  Type=simple
> >  Environment=XENCONSOLED_ARGS=
> > -Environment=XENCONSOLED_LOG=none
> > +Environment=XENCONSOLED_TRACE=none
> >  Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
> >  EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
> >  PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
> >  ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
> >  ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}
> > -ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_LOG} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
> > +ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_TRACE} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
> >  
> >  [Install]
> >  WantedBy=multi-user.target
> 
> 

Re: [PATCH 5/7] tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service

[Ian Jackson]

Olaf Hering writes ("[PATCH 5/7] tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service"):
> Instead of inventing a new XENCONSOLED_LOG= variable reuse the
> existing XENCONSOLED_TRACE= variable in xenconsoled.service.

This still results in a duplication of the default value `none': one
in xencommons and one in the systemd service file.

But it is a step in the right direction so

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

[PATCH 6/7] tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service

[Olaf Hering]

The references Environment file does not exist, and the service file
does not make use of variables anyway.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
index 0a5807a..274cec0 100644
--- a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
+++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
@@ -8,7 +8,6 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=simple
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
 PIDFile=@XEN_RUN_DIR@/qemu-dom0.pid
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@

Re: [PATCH 6/7] tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service

[Ian Campbell]

On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> The references Environment file does not exist, and the service file

"referenced"

> does not make use of variables anyway.

In principal the env vars are also for the benefit of the thing launched
by the Exec line too, not just the unit file.

qemu-system-i386 does use getenv() but not in any way which looks
relevant under Xen (e.g. linux-user/main.c uses it, but we don't care).

If someone changes exec to something (e.g. a wrapper) which cares about
envvars they can equally well set EnvironmentFile too. So:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 6/7] tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service

[Ian Jackson]

Olaf Hering writes ("[PATCH 6/7] tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service"):
> The references Environment file does not exist, and the service file
> does not make use of variables anyway.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Although of course this will have to come back in a slightly different
form if we ever start honouring env settings for any reason.

Thanks,
Ian.

[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 <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@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/xencom
 mons"
+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 $(XEN_ROOT)/tools/Rules.mk
 
 # Init scripts.
+XENSTORED_LIBEXEC = xenstored.sh
 XENDOMAINS_INITD = init.d/xendomains
 XENDOMAINS_LIBEXEC = xendomains
 XENDOMAINS_SYSCONFIG = init.d/sysconfig.xendomains
@@ -51,6 +52,7 @@ install-initd:
 	[ -d $(DESTDIR)$(INITD_DIR) ] || $(INSTALL_DIR) $(DESTDIR)$(INITD_DIR)
 	[ -d $(DESTDIR)$(SYSCONFIG_DIR) ] || $(INSTALL_DIR) $(DESTDIR)$(SYSCONFIG_DIR)
 	[ -d $(DESTDIR)$(LIBEXEC_BIN) ] || $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_PROG) $(XENDOMAINS_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_PROG) $(XENDOMAINS_INITD) $(DESTDIR)$(INITD_DIR)
 	$(INSTALL_DATA) $(XENDOMAINS_SYSCONFIG) $(DESTDIR)$(SYSCONFIG_DIR)/xendomains
diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index a1095c2..f57bfd3 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -66,11 +66,13 @@ do_start () {
 	then
 		test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
 		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
-		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
 
 		if [ -n "$XENSTORED" ] ; then
 		    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
 		else
 		    echo "No xenstored found"
 		    exit 1
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 0f0ac58..a048b21 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -8,13 +8,12 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=notify
-Environment=XENSTORED_ARGS=
 Environment=XENSTORED=@XENSTORED@
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
+EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
 ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
-ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
+ExecStart=@LIBEXEC_BIN@/xenstored.sh --no-fork
 
 [Install]
 WantedBy=multi-user.target
diff --git a/tools/hotplug/Linux/xenstored.sh.in b/tools/hotplug/Linux/xenstored.sh.in
new file mode 100644
index 0000000..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

Re: [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.

Re: [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

Re: [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.

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

[Konrad Rzeszutek Wilk]

On Wed, Jan 07, 2015 at 03:27:15PM +0000, 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.

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

[George Dunlap]

On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering <olaf@aepfle.de> 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

Re: [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 <olaf@aepfle.de> 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

Re: [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 <olaf@aepfle.de> 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

Re: [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 <olaf@aepfle.de> 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

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

[George Dunlap]

On Thu, Sep 10, 2015 at 4:01 PM, M A Young <m.a.young@durham.ac.uk> 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@aepfle.de> 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=c000003e 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

Re: [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.

Re: [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

Re: [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.jackson@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 0000000..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.

Re: [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.jackson@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 0000000..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

Re: [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.jackson@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 0000000..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 :-)

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On Fri, Dec 19, 2014 at 12:25:26PM +0100, Olaf Hering wrote:
> This is a resend of these two series:
> http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00858.html
> http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html
> 
> New in v3 is a wrapper to run xenstored. See its patch description
> for details.
> 
> Patch 2-6 should be applied for 4.5.0.
> 
> The first and the last one still has issues with xenstored and
> SELinux. See below.  Up to now no solution is known to me.
> 
> 
> The first patch fixes Arch Linux and does not break anything.  As such
> it should be safe to be applied for 4.5.0.  SELinux users (who build
> from source) should put their special mount options into fstab. Distro

Could you elaborate what that is? As in what is that 'special mount options'?

> packages will most likely include a proper .service file.
> 
> 
> The last patch addresses the XENSTORED_TRACE issue. But SELinux will
> most likely still not work.
> 
> Possible ways to handle launching xenstored and SELinux:
> 
> - do nothing
>   pro: - no Xen source changes required
>   con: - possible unhappy users who build from source and still have
>          SELinux enabled

At this stage I prefer this and just have in the release notes the
work-around documented.
> 
> - use newly added wrapper
>   pro: - XENSTORED_TRACE boolean is handled
>   con: - the wrapper may have the very same issue as the current
>          launching with sh -c 'exec xenstored'. But maybe there is a
> 	 way to mark the new wrapper script as "this is the native
> 	 xenstored". Someone familiar with SELinux may be able to
> 	 answer this.
> 
> - Use ExecStart=@XENSTORED@
>   pro: - socket passing will most likely work
>   con: - All options have to be passed in XENSTORED_ARGS, a new variable
>          which is not yet mentioned in the sysconfig file.
>        - Switching xenstored requires a private copy of
> 	 xenstored.service in /etc/systemd instead of adjusting the
> 	 XENSTORED= variable in the sysconfig file.
> 
> - Use ExecStart=/usr/bin/env $XENSTORED
>   pro: - $XENSTORED can be set in sysconfig file
>   con: - may have the same socket issue as starting via shell
>        - XENSTORED_TRACE boolean is not handled
> 
> 
> I will be offline until 2015-01-07, so any further adjustments to this
> series has to be done by someone else.
> 
> 
> Good luck!
> 
> Olaf
> 
> 
> Olaf Hering (7):
>   tools/hotplug: remove SELinux options from var-lib-xenstored.mount
>   tools/hotplug: remove XENSTORED_ROOTDIR from xenstored.service
>   tools/hotplug: xendomains.service depends on network
>   tools/hotplug: use xencommons as EnvironmentFile in
>     xenconsoled.service
>   tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service
>   tools/hotplug: remove EnvironmentFile from
>     xen-qemu-dom0-disk-backend.service
>   tools/hotplug: add wrapper to start xenstored
> 
>  .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/var-lib-xenstored.mount.in            | 4 +---
>  tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 -
>  tools/hotplug/Linux/systemd/xenconsoled.service.in                | 6 +++---
>  tools/hotplug/Linux/systemd/xendomains.service.in                 | 2 ++
>  tools/hotplug/Linux/systemd/xenstored.service.in                  | 6 ++----
>  tools/hotplug/Linux/xenstored.sh.in                               | 6 ++++++
>  11 files changed, 24 insertions(+), 14 deletions(-)
>  create mode 100644 tools/hotplug/Linux/xenstored.sh.in
> 

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

On Fri, Dec 19, Konrad Rzeszutek Wilk wrote:

> On Fri, Dec 19, 2014 at 12:25:26PM +0100, Olaf Hering wrote:
> > This is a resend of these two series:
> > http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00858.html
> > http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html
> > 
> > New in v3 is a wrapper to run xenstored. See its patch description
> > for details.
> > 
> > Patch 2-6 should be applied for 4.5.0.
> > 
> > The first and the last one still has issues with xenstored and
> > SELinux. See below.  Up to now no solution is known to me.
> > 
> > 
> > The first patch fixes Arch Linux and does not break anything.  As such
> > it should be safe to be applied for 4.5.0.  SELinux users (who build
> > from source) should put their special mount options into fstab. Distro
> 
> Could you elaborate what that is? As in what is that 'special mount options'?

The context= mount option, about which we argue since a few weeks?
See patch #1.

> > packages will most likely include a proper .service file.
> > 
> > 
> > The last patch addresses the XENSTORED_TRACE issue. But SELinux will
> > most likely still not work.
> > 
> > Possible ways to handle launching xenstored and SELinux:
> > 
> > - do nothing
> >   pro: - no Xen source changes required
> >   con: - possible unhappy users who build from source and still have
> >          SELinux enabled
> 
> At this stage I prefer this and just have in the release notes the
> work-around documented.

Which workaround is that? No SELinux on Fedora?

Olaf

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On Mon, Dec 22, 2014 at 09:06:40AM +0100, Olaf Hering wrote:
> On Fri, Dec 19, Konrad Rzeszutek Wilk wrote:
> 
> > On Fri, Dec 19, 2014 at 12:25:26PM +0100, Olaf Hering wrote:
> > > This is a resend of these two series:
> > > http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00858.html
> > > http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html
> > > 
> > > New in v3 is a wrapper to run xenstored. See its patch description
> > > for details.
> > > 
> > > Patch 2-6 should be applied for 4.5.0.
> > > 
> > > The first and the last one still has issues with xenstored and
> > > SELinux. See below.  Up to now no solution is known to me.
> > > 
> > > 
> > > The first patch fixes Arch Linux and does not break anything.  As such
> > > it should be safe to be applied for 4.5.0.  SELinux users (who build
> > > from source) should put their special mount options into fstab. Distro
> > 
> > Could you elaborate what that is? As in what is that 'special mount options'?
> 
> The context= mount option, about which we argue since a few weeks?

You said 'special mount options into fstab' ? Is that the same as 'context='??
(checks the manpage) AHA, it is!


In which case would it just to say that this needs to be added as
a workaround:

xenstored /var/lib/xenstored xenstored context="system_u:object_r:xenstored_var_lib_t:s0" 1 1

> See patch #1.
> 
> > > packages will most likely include a proper .service file.
> > > 
> > > 
> > > The last patch addresses the XENSTORED_TRACE issue. But SELinux will
> > > most likely still not work.
> > > 
> > > Possible ways to handle launching xenstored and SELinux:
> > > 
> > > - do nothing
> > >   pro: - no Xen source changes required
> > >   con: - possible unhappy users who build from source and still have
> > >          SELinux enabled
> > 
> > At this stage I prefer this and just have in the release notes the
> > work-around documented.
> 
> Which workaround is that? No SELinux on Fedora?

That is not an option.

The workaround is to document what the 'context' is .. or whatever
else is needed to make this work.

> 
> Olaf

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On Wed, Dec 31, 2014 at 10:31:06AM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 22, 2014 at 09:06:40AM +0100, Olaf Hering wrote:
> > On Fri, Dec 19, Konrad Rzeszutek Wilk wrote:
> > 
> > > On Fri, Dec 19, 2014 at 12:25:26PM +0100, Olaf Hering wrote:
> > > > This is a resend of these two series:
> > > > http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00858.html
> > > > http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html
> > > > 
> > > > New in v3 is a wrapper to run xenstored. See its patch description
> > > > for details.
> > > > 
> > > > Patch 2-6 should be applied for 4.5.0.

IanJ, Wei, IanC, please read below.

Patch #2-#6:

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

#2,#3 has an Ack

#4 ("tools/hotplug: use xencommons as EnvironmentFile in xenconsoled.service")
#5 ("tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service")
#6 ("tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service")

need Acks. 

> > > > 
> > > > The first and the last one still has issues with xenstored and
> > > > SELinux. See below.  Up to now no solution is known to me.
> > > > 
> > > > 
> > > > The first patch fixes Arch Linux and does not break anything.  As such
> > > > it should be safe to be applied for 4.5.0.  SELinux users (who build
> > > > from source) should put their special mount options into fstab. Distro

For patch #1 ("tools/hotplug: remove SELinux options from var-lib-xenstored.mount")

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

with the below change to README file. It also needs an Ack.

For patch #7 (" tools/hotplug: add wrapper to start xenstored")

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
However there is a question in there for Ian:

"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".
"

IanJ - any specific reasons for having it in /etc instead of
LIBEXEC_BIN? This is in regards to the introduction of this file:

	diff --git a/tools/hotplug/Linux/xenstored.sh.in b/tools/hotplug/Linux/xenstored.sh.in
	new file mode 100644
	index 0000000..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


> > > 
> > > Could you elaborate what that is? As in what is that 'special mount options'?
> > 
> > The context= mount option, about which we argue since a few weeks?
> 
> You said 'special mount options into fstab' ? Is that the same as 'context='??
> (checks the manpage) AHA, it is!
> 
> 
> In which case would it just to say that this needs to be added as
> a workaround:
> 
> xenstored /var/lib/xenstored xenstored context="system_u:object_r:xenstored_var_lib_t:s0" 1 1

To be exact:

tmpfs                   /var/lib/xenstored      tmpfs   mode=755,context="system_u:object_r:xenstored_var_lib_t:s0" 0 0

> 
> > See patch #1.
> > 
> > > > packages will most likely include a proper .service file.
> > > > 
> > > > 
> > > > The last patch addresses the XENSTORED_TRACE issue. But SELinux will
> > > > most likely still not work.
> > > > 
> > > > Possible ways to handle launching xenstored and SELinux:
> > > > 
> > > > - do nothing
> > > >   pro: - no Xen source changes required
> > > >   con: - possible unhappy users who build from source and still have
> > > >          SELinux enabled
> > > 
> > > At this stage I prefer this and just have in the release notes the
> > > work-around documented.
> > 
> > Which workaround is that? No SELinux on Fedora?
> 
> That is not an option.
> 
> The workaround is to document what the 'context' is .. or whatever
> else is needed to make this work.

Such as this might be good (Or perhaps move it to the INSTALL file)

diff --git a/README b/README
index 412607a..7d74214 100644
--- a/README
+++ b/README
@@ -33,6 +33,26 @@ This file contains some quick-start instructions to install Xen on
 your system. For more information see http:/www.xen.org/ and
 http://wiki.xen.org/
 
+Release Issues
+==============
+
+While we did the utmost to get a release out, there are certain
+fixes which were not complete on time. As such please reference this
+section if you are running into trouble.
+
+    * systemd not working with Fedora Core 20, 21 or later (systemctl
+      reports xenstore failing to start).
+
+      Systemd support is now part of Xen source code. While utmost work has
+      been done to make the systemd files compatible across all the
+      distributions, there might issues when using systemd files from
+      Xen sources. The work-around is to define an mount entry in
+      /etc/fstab as follow:
+
+      tmpfs                   /var/lib/xenstored      tmpfs
+      mode=755,context="system_u:object_r:xenstored_var_lib_t:s0" 0 0
+
+
 Quick-Start Guide
 =================
 
> 
> > 
> > Olaf
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Ian Campbell]

On Mon, 2015-01-05 at 16:22 -0500, Konrad Rzeszutek Wilk wrote:
> However there is a question in there for Ian:
> 
> "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".
> "
> 
> IanJ - any specific reasons for having it in /etc instead of
> LIBEXEC_BIN? 

IIRC Ian explained this is the course of the thread. It's because an
administrator might reasonably want to edit the file to apply local
configuration. It is in effect a configuration file masquerading as a
script.

> > The workaround is to document what the 'context' is .. or whatever
> > else is needed to make this work.
> 
> Such as this might be good (Or perhaps move it to the INSTALL file)

I think the Release Notes are the right place for this sort of
information. e.g.
http://wiki.xenproject.org/wiki/Xen_Project_4.4_Release_Notes#Known_issues

Ian.

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5"):
> #4 ("tools/hotplug: use xencommons as EnvironmentFile in xenconsoled.service")
> #5 ("tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service")
> #6 ("tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service")
> 
> need Acks. 

Done.

> For patch #1 ("tools/hotplug: remove SELinux options from var-lib-xenstored.mount")
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> with the below change to README file. It also needs an Ack.

Done.

> For patch #7 (" tools/hotplug: add wrapper to start xenstored")
> 
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> However there is a question in there for Ian:
> 
> "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".
> "
> 
> IanJ - any specific reasons for having it in /etc instead of
> LIBEXEC_BIN? This is in regards to the introduction of this file:

I explained this in my previous response and made what I thought was
an unequivocal declaration about the location of the file.

> Such as this might be good (Or perhaps move it to the INSTALL file)
...
> --- a/README
> +++ b/README
...
> +Release Issues
> +==============

I'm happy to have this particular issue here in the README.

But I think the release notes need to be out of tree.  This is so that
if we discover an issue between last commit deadline and release, we
can update the release notes.

Thanks,
Ian.

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On Tue, Jan 06, 2015 at 03:00:16PM +0000, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5"):
> > #4 ("tools/hotplug: use xencommons as EnvironmentFile in xenconsoled.service")
> > #5 ("tools/hotplug: use XENCONSOLED_TRACE in xenconsoled.service")
> > #6 ("tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service")
> > 
> > need Acks. 
> 
> Done.

Thank you. Let me apply #1-#6 in staging then.
> 
> > For patch #1 ("tools/hotplug: remove SELinux options from var-lib-xenstored.mount")
> > 
> > Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > with the below change to README file. It also needs an Ack.
> 
> Done.
> 
> > For patch #7 (" tools/hotplug: add wrapper to start xenstored")
> > 
> > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > However there is a question in there for Ian:
> > 
> > "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".
> > "
> > 
> > IanJ - any specific reasons for having it in /etc instead of
> > LIBEXEC_BIN? This is in regards to the introduction of this file:
> 
> I explained this in my previous response and made what I thought was
> an unequivocal declaration about the location of the file.
> 
> > Such as this might be good (Or perhaps move it to the INSTALL file)
> ...
> > --- a/README
> > +++ b/README
> ...
> > +Release Issues
> > +==============
> 
> I'm happy to have this particular issue here in the README.
> 
> But I think the release notes need to be out of tree.  This is so that
> if we discover an issue between last commit deadline and release, we
> can update the release notes.

<nods> Will create one on the Wiki and add it there.
> 
> Thanks,
> Ian.

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

On Mon, Jan 05, Konrad Rzeszutek Wilk wrote:

> +Release Issues
> +==============
> +
> +While we did the utmost to get a release out, there are certain
> +fixes which were not complete on time. As such please reference this
> +section if you are running into trouble.
> +
> +    * systemd not working with Fedora Core 20, 21 or later (systemctl
> +      reports xenstore failing to start).
> +
> +      Systemd support is now part of Xen source code. While utmost work has
> +      been done to make the systemd files compatible across all the
> +      distributions, there might issues when using systemd files from
> +      Xen sources. The work-around is to define an mount entry in
> +      /etc/fstab as follow:
> +
> +      tmpfs                   /var/lib/xenstored      tmpfs
> +      mode=755,context="system_u:object_r:xenstored_var_lib_t:s0" 0 0
> +
> +

Shouldnt this go into a new SELinux section in the INSTALL file?

Its my understanding that the reported SELinux failure is not only
related to the context= mount option, but also to the socket passing
from systemd.


Olaf

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On Wed, Jan 07, 2015 at 10:53:06AM +0100, Olaf Hering wrote:
> On Mon, Jan 05, Konrad Rzeszutek Wilk wrote:
> 
> > +Release Issues
> > +==============
> > +
> > +While we did the utmost to get a release out, there are certain
> > +fixes which were not complete on time. As such please reference this
> > +section if you are running into trouble.
> > +
> > +    * systemd not working with Fedora Core 20, 21 or later (systemctl
> > +      reports xenstore failing to start).
> > +
> > +      Systemd support is now part of Xen source code. While utmost work has
> > +      been done to make the systemd files compatible across all the
> > +      distributions, there might issues when using systemd files from
> > +      Xen sources. The work-around is to define an mount entry in
> > +      /etc/fstab as follow:
> > +
> > +      tmpfs                   /var/lib/xenstored      tmpfs
> > +      mode=755,context="system_u:object_r:xenstored_var_lib_t:s0" 0 0
> > +
> > +
> 
> Shouldnt this go into a new SELinux section in the INSTALL file?

It is going in the web-page for 'Release Issues' and such.
> 
> Its my understanding that the reported SELinux failure is not only
> related to the context= mount option, but also to the socket passing
> from systemd.


I couldn't spot any errors in SELinux for this. Perhaps I had misconfigured?
> 
> 
> Olaf

Re: [PATCH 0/7 v3] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

On Wed, Jan 07, Konrad Rzeszutek Wilk wrote:

> On Wed, Jan 07, 2015 at 10:53:06AM +0100, Olaf Hering wrote:
> > Its my understanding that the reported SELinux failure is not only
> > related to the context= mount option, but also to the socket passing
> > from systemd.
> 
> I couldn't spot any errors in SELinux for this. Perhaps I had misconfigured?

Last year you said xenstored did not start, even with patch #1 applied.
I dont know if you added the required fstab changes. So if current
staging works fine with SELinux enabled we could go with this change for
the service file, instead of the wrapper:

ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS


Does that work for you? If yes, lets get rid of the XENSTORED_TRACE=
boolean and use a new XENSTORED_ARGS= variable instead. That would make
patch #7 alot simpler.

Olaf