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

[Daniel Veillard]

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.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/