Re: [libvirt] [PATCH] (v2) avoid chowning domain devices if higer-level mgmt does it for us

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.