[libvirt] [PATCH for 5.0.0] qemu: Temporary disable owner remembering

Michal Privoznik posted 1 patch 5 years, 3 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/b77652afb1679fdf0b6506c9a76f0039bc82a5e3.1547485024.git.mprivozn@redhat.com
docs/news.xml                      | 13 -------------
src/qemu/libvirtd_qemu.aug         |  1 -
src/qemu/qemu.conf                 |  5 -----
src/qemu/qemu_conf.c               |  4 ----
src/qemu/test_libvirtd_qemu.aug.in |  1 -
5 files changed, 24 deletions(-)
[libvirt] [PATCH for 5.0.0] qemu: Temporary disable owner remembering
Posted by Michal Privoznik 5 years, 3 months ago
Turns out, that there are few bugs that are not that trivial to
fix (e.g. around block jobs). Instead of rushing in not
thoroughly tested fixes disable the feature temporarily for the
release.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/news.xml                      | 13 -------------
 src/qemu/libvirtd_qemu.aug         |  1 -
 src/qemu/qemu.conf                 |  5 -----
 src/qemu/qemu_conf.c               |  4 ----
 src/qemu/test_libvirtd_qemu.aug.in |  1 -
 5 files changed, 24 deletions(-)

diff --git a/docs/news.xml b/docs/news.xml
index 90b7e8891e..c7a4dde463 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -66,19 +66,6 @@
           qemu: Add support for ARMv6l guests
         </summary>
       </change>
-      <change>
-        <summary>
-          Remember original owners and SELinux labels of files
-        </summary>
-        <description>
-          When a domain is starting up libvirt changes DAC and
-          SELinux labels so that domain can access it. However,
-          it never remembered the original labels and therefore
-          the file was returned back to <code>root:root</code>.
-          With this release, the original labels are remembered
-          and restored properly.
-        </description>
-      </change>
       <change>
         <summary>
           Support more NVDIMM configuration options
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 8a5b39e568..ddc4bbfd1d 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -71,7 +71,6 @@ module Libvirtd_qemu =
                  | str_entry "user"
                  | str_entry "group"
                  | bool_entry "dynamic_ownership"
-                 | bool_entry "remember_owner"
                  | str_array_entry "cgroup_controllers"
                  | str_array_entry "cgroup_device_acl"
                  | int_entry "seccomp_sandbox"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 29093f6329..28e51b2c59 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -450,11 +450,6 @@
 # Set to 0 to disable file ownership changes.
 #dynamic_ownership = 1
 
-# Whether libvirt should remember and restore the original
-# ownership over files it is relabeling. Defaults to 1, set
-# to 0 to disable the feature.
-#remember_owner = 1
-
 # What cgroup controllers to make use of with QEMU guests
 #
 #  - 'cpu' - use for scheduler tunables
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 20952e9607..b03e38b831 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -145,7 +145,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
         cfg->group = (gid_t)-1;
     }
     cfg->dynamicOwnership = privileged;
-    cfg->rememberOwner = privileged;
 
     cfg->cgroupControllers = -1; /* -1 == auto-detect */
 
@@ -729,9 +728,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
     if (virConfGetValueBool(conf, "dynamic_ownership", &cfg->dynamicOwnership) < 0)
         goto cleanup;
 
-    if (virConfGetValueBool(conf, "remember_owner", &cfg->rememberOwner) < 0)
-        goto cleanup;
-
     if (virConfGetValueStringList(conf,  "cgroup_controllers", false,
                                   &controllers) < 0)
         goto cleanup;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 92a8ae1192..f1e8806ad2 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -43,7 +43,6 @@ module Test_libvirtd_qemu =
 { "user" = "root" }
 { "group" = "root" }
 { "dynamic_ownership" = "1" }
-{ "remember_owner" = "1" }
 { "cgroup_controllers"
     { "1" = "cpu" }
     { "2" = "devices" }
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH for 5.0.0] qemu: Temporary disable owner remembering
Posted by Peter Krempa 5 years, 3 months ago
On Mon, Jan 14, 2019 at 17:57:39 +0100, Michal Privoznik wrote:
> Turns out, that there are few bugs that are not that trivial to
> fix (e.g. around block jobs). Instead of rushing in not
> thoroughly tested fixes disable the feature temporarily for the
> release.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/news.xml                      | 13 -------------
>  src/qemu/libvirtd_qemu.aug         |  1 -
>  src/qemu/qemu.conf                 |  5 -----
>  src/qemu/qemu_conf.c               |  4 ----
>  src/qemu/test_libvirtd_qemu.aug.in |  1 -
>  5 files changed, 24 deletions(-)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 90b7e8891e..c7a4dde463 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -66,19 +66,6 @@
>            qemu: Add support for ARMv6l guests
>          </summary>
>        </change>
> -      <change>
> -        <summary>
> -          Remember original owners and SELinux labels of files
> -        </summary>
> -        <description>
> -          When a domain is starting up libvirt changes DAC and
> -          SELinux labels so that domain can access it. However,
> -          it never remembered the original labels and therefore
> -          the file was returned back to <code>root:root</code>.
> -          With this release, the original labels are remembered
> -          and restored properly.
> -        </description>
> -      </change>
>        <change>
>          <summary>
>            Support more NVDIMM configuration options
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index 8a5b39e568..ddc4bbfd1d 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -71,7 +71,6 @@ module Libvirtd_qemu =
>                   | str_entry "user"
>                   | str_entry "group"
>                   | bool_entry "dynamic_ownership"
> -                 | bool_entry "remember_owner"
>                   | str_array_entry "cgroup_controllers"
>                   | str_array_entry "cgroup_device_acl"
>                   | int_entry "seccomp_sandbox"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 29093f6329..28e51b2c59 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -450,11 +450,6 @@
>  # Set to 0 to disable file ownership changes.
>  #dynamic_ownership = 1
>  
> -# Whether libvirt should remember and restore the original
> -# ownership over files it is relabeling. Defaults to 1, set
> -# to 0 to disable the feature.
> -#remember_owner = 1
> -
>  # What cgroup controllers to make use of with QEMU guests
>  #
>  #  - 'cpu' - use for scheduler tunables
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 20952e9607..b03e38b831 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -145,7 +145,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
>          cfg->group = (gid_t)-1;
>      }
>      cfg->dynamicOwnership = privileged;
> -    cfg->rememberOwner = privileged;

Can't we just set this to false and add a note to the qemu.conf file
that it's currently disabled as being unstable so that you don't have to
delete the qemu_conf.c changes?

ACK for that option.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH for 5.0.0] qemu: Temporary disable owner remembering
Posted by Daniel P. Berrangé 5 years, 3 months ago
On Mon, Jan 14, 2019 at 06:01:24PM +0100, Peter Krempa wrote:
> On Mon, Jan 14, 2019 at 17:57:39 +0100, Michal Privoznik wrote:
> > Turns out, that there are few bugs that are not that trivial to
> > fix (e.g. around block jobs). Instead of rushing in not
> > thoroughly tested fixes disable the feature temporarily for the
> > release.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >  docs/news.xml                      | 13 -------------
> >  src/qemu/libvirtd_qemu.aug         |  1 -
> >  src/qemu/qemu.conf                 |  5 -----
> >  src/qemu/qemu_conf.c               |  4 ----
> >  src/qemu/test_libvirtd_qemu.aug.in |  1 -
> >  5 files changed, 24 deletions(-)
> > 
> > diff --git a/docs/news.xml b/docs/news.xml
> > index 90b7e8891e..c7a4dde463 100644
> > --- a/docs/news.xml
> > +++ b/docs/news.xml
> > @@ -66,19 +66,6 @@
> >            qemu: Add support for ARMv6l guests
> >          </summary>
> >        </change>
> > -      <change>
> > -        <summary>
> > -          Remember original owners and SELinux labels of files
> > -        </summary>
> > -        <description>
> > -          When a domain is starting up libvirt changes DAC and
> > -          SELinux labels so that domain can access it. However,
> > -          it never remembered the original labels and therefore
> > -          the file was returned back to <code>root:root</code>.
> > -          With this release, the original labels are remembered
> > -          and restored properly.
> > -        </description>
> > -      </change>
> >        <change>
> >          <summary>
> >            Support more NVDIMM configuration options
> > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> > index 8a5b39e568..ddc4bbfd1d 100644
> > --- a/src/qemu/libvirtd_qemu.aug
> > +++ b/src/qemu/libvirtd_qemu.aug
> > @@ -71,7 +71,6 @@ module Libvirtd_qemu =
> >                   | str_entry "user"
> >                   | str_entry "group"
> >                   | bool_entry "dynamic_ownership"
> > -                 | bool_entry "remember_owner"
> >                   | str_array_entry "cgroup_controllers"
> >                   | str_array_entry "cgroup_device_acl"
> >                   | int_entry "seccomp_sandbox"
> > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> > index 29093f6329..28e51b2c59 100644
> > --- a/src/qemu/qemu.conf
> > +++ b/src/qemu/qemu.conf
> > @@ -450,11 +450,6 @@
> >  # Set to 0 to disable file ownership changes.
> >  #dynamic_ownership = 1
> >  
> > -# Whether libvirt should remember and restore the original
> > -# ownership over files it is relabeling. Defaults to 1, set
> > -# to 0 to disable the feature.
> > -#remember_owner = 1
> > -
> >  # What cgroup controllers to make use of with QEMU guests
> >  #
> >  #  - 'cpu' - use for scheduler tunables
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index 20952e9607..b03e38b831 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -145,7 +145,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> >          cfg->group = (gid_t)-1;
> >      }
> >      cfg->dynamicOwnership = privileged;
> > -    cfg->rememberOwner = privileged;
> 
> Can't we just set this to false and add a note to the qemu.conf file
> that it's currently disabled as being unstable so that you don't have to
> delete the qemu_conf.c changes?

If we do that, then people can turn it on. Then we have to ensure we can
handle upgrades correctly.  If it is buggy, it is safer to never let users
turn it on so we don't get ourselves into a painful place with upgrades
from the buggy version

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH for 5.0.0] qemu: Temporary disable owner remembering
Posted by Peter Krempa 5 years, 3 months ago
On Mon, Jan 14, 2019 at 17:05:50 +0000, Daniel Berrange wrote:
> On Mon, Jan 14, 2019 at 06:01:24PM +0100, Peter Krempa wrote:
> > On Mon, Jan 14, 2019 at 17:57:39 +0100, Michal Privoznik wrote:
> > > Turns out, that there are few bugs that are not that trivial to
> > > fix (e.g. around block jobs). Instead of rushing in not
> > > thoroughly tested fixes disable the feature temporarily for the
> > > release.
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

[...]

> > > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > > index 20952e9607..b03e38b831 100644
> > > --- a/src/qemu/qemu_conf.c
> > > +++ b/src/qemu/qemu_conf.c
> > > @@ -145,7 +145,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> > >          cfg->group = (gid_t)-1;
> > >      }
> > >      cfg->dynamicOwnership = privileged;
> > > -    cfg->rememberOwner = privileged;
> > 
> > Can't we just set this to false and add a note to the qemu.conf file
> > that it's currently disabled as being unstable so that you don't have to
> > delete the qemu_conf.c changes?
> 
> If we do that, then people can turn it on. Then we have to ensure we can
> handle upgrades correctly.  If it is buggy, it is safer to never let users
> turn it on so we don't get ourselves into a painful place with upgrades
> from the buggy version

I would not feel bad for anybody enabling a option which is clearly
marked as broken which would make their stuff blow up.

Anyways.

ACK even to the original patch since I don't care that much.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list