On Wed, Nov 25, 2009 at 10:31:03AM +0100, Daniel Veillard wrote:
> On Wed, Nov 25, 2009 at 09:27:13AM +0200, Dan Kenigsberg wrote:
> > this is particularily important if said device is a file sitting on a
> > root_squashing nfs export.
> >
> > my previous attempt for a patch missed 3 chowns that should be avoided.
> > ---
> > src/qemu/qemu.conf | 4 ++++
> > src/qemu/qemu_conf.c | 3 +++
> > src/qemu/qemu_conf.h | 1 +
> > src/qemu/qemu_driver.c | 8 ++++----
> > 4 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> > index bca858a..892a50b 100644
> > --- a/src/qemu/qemu.conf
> > +++ b/src/qemu/qemu.conf
> > @@ -96,6 +96,10 @@
> > # The group ID for QEMU processes run by the system instance
> > #group = "root"
> >
> > +# should libvirt assume that devices are accessible to the above user:group.
> > +# by default, libvirt tries to chown devices before starting up a domain and
> > +# restore ownership to root when domain comes down.
> > +#assume_devices_accessible = 0
> >
> > # What cgroup controllers to make use of with QEMU guests
> > #
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index b1b9e5f..520a395 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -232,6 +232,9 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
> > return -1;
> > }
> >
> > + p = virConfGetValue (conf, "assume_devices_accessible");
> > + CHECK_TYPE ("assume_devices_accessible", VIR_CONF_LONG);
> > + if (p) driver->avoid_dev_chown = p->l;
>
> an explicit initialization of the field would be better if p is NULL.
>
> > if (virGetGroupID(NULL, group, &driver->group) < 0) {
> > VIR_FREE(group);
> > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> > index 675c636..3a9da73 100644
> > --- a/src/qemu/qemu_conf.h
> > +++ b/src/qemu/qemu_conf.h
> > @@ -87,6 +87,7 @@ struct qemud_driver {
> >
> > uid_t user;
> > gid_t group;
> > + int avoid_dev_chown;
> >
> > unsigned int qemuVersion;
> > int nextvmid;
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 2f273eb..5f02aa2 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -1968,7 +1968,7 @@ static int qemuDomainSetDeviceOwnership(virConnectPtr conn,
> > uid_t uid;
> > gid_t gid;
> >
> > - if (!driver->privileged)
> > + if (!driver->privileged || driver->avoid_dev_chown)
> > return 0;
> >
> > /* short circuit case of root:root */
> > @@ -2002,7 +2002,7 @@ static int qemuDomainSetAllDeviceOwnership(virConnectPtr conn,
> > uid_t uid;
> > gid_t gid;
> >
> > - if (!driver->privileged)
> > + if (!driver->privileged || driver->avoid_dev_chown)
> > return 0;
> >
> > /* short circuit case of root:root */
> > @@ -3438,7 +3438,7 @@ static int qemudDomainSave(virDomainPtr dom,
> > }
> > fd = -1;
> >
> > - if (driver->privileged &&
> > + if (driver->privileged && !driver->avoid_dev_chown &&
> > chown(path, driver->user, driver->group) < 0) {
> > virReportSystemError(NULL, errno,
> > _("unable to set ownership of '%s' to user %d:%d"),
> > @@ -3473,7 +3473,7 @@ static int qemudDomainSave(virDomainPtr dom,
> > if (rc < 0)
> > goto endjob;
> >
> > - if (driver->privileged &&
> > + if (driver->privileged && !driver->avoid_dev_chown &&
> > chown(path, 0, 0) < 0) {
> > virReportSystemError(NULL, errno,
> > _("unable to set ownership of '%s' to user %d:%d"),
>
> The core question is having this as another manual user tweak, it always
> makes me a bit uncomfortable if proper working of software requires manually
> editing a config file. If we really need this kind of options for proper
> operations in specific conditions can we make sure it can be set via
> APIs too ? I don't think we should expose something as generic as the
> internal Conf APIs, but these runtime option in src/qemu/qemu.conf
> start to accumulate and I start to wonder how to properly manage all
> this.
Aren't we here to babysit software? :-)
For this particular behavior, I personally feel that the default is
wrong. I don't like it that libvirt takes the liberty to (try to) change
ownership, I like it less when it does not truely restore ownership, and
even less, that it fails to use files over nfs.
I understand that this behavior was meant to assist admins who upgrade
from running qemu as root to running it as a meager user. But personally
I believe that we could have politely asked them: if you use this new
feature, please make sure this new qemu user has access to your
files/devices. Now that there are people expecting current behavior, I
don't really see a nice way out.
Dan
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list