[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Change default for storage uid/gid from getuid()/getgid() to -1/-1



On Tue, Mar 02, 2010 at 02:17:59AM -0500, Laine Stump wrote:
> This allows the config to have a setting that means "leave it alone",
> eg when building a pool where the directory already exists the user
> may want the current uid/gid of the directory left intact. This
> actually gets us back to older behavior - before recent changes to the
> pool building code, we weren't as insistent about honoring the uid/gid
> settings in the XML, and virt-manager was taking advantage of this
> behavior.
> 
> As a side benefit, removing calls to getuid/getgid from the XML
> parsing functions also seems like a good idea. And having a default
> that is different from a common/useful value (0 == root) is a good
> thing in general, as it removes ambiguity from decisions (at least one
> place in the code was checking for (perms.uid == 0) to see if a
> special uid was requested).
> 
> Note that this will only affect newly created pools and volumes. Due
> to the way that the XML is parsed, then formatted for newly created
> volumes, all existing pools/volumes already have an explicit uid and
> gid set.
> 
> src/conf/storage_conf.c: Remove calls to setuid/setgid for default values
>                          of uid/gid, and set them to -1 instead
> 
> src/storage/storage_backend.c:
> src/storage/storage_backend_fs.c:
>         Make account for the new default values of perms.uid
>         and perms.gid.
> ---
>  src/conf/storage_conf.c          |    8 +++---
>  src/storage/storage_backend.c    |   25 +++++++++++++----------
>  src/storage/storage_backend_fs.c |   39 +++++++++++++++++++++++++++----------
>  3 files changed, 46 insertions(+), 26 deletions(-)
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 19a1db9..f59f109 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -539,8 +539,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>      if (node == NULL) {
>          /* Set default values if there is not <permissions> element */
>          perms->mode = defaultmode;
> -        perms->uid = getuid();
> -        perms->gid = getgid();
> +        perms->uid = -1;
> +        perms->gid = -1;
>          perms->label = NULL;
>          return 0;
>      }
> @@ -564,7 +564,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>      }
>  
>      if (virXPathNode("./owner", ctxt) == NULL) {
> -        perms->uid = getuid();
> +        perms->uid = -1;
>      } else {
>          if (virXPathLong("number(./owner)", ctxt, &v) < 0) {
>              virStorageReportError(VIR_ERR_XML_ERROR,
> @@ -575,7 +575,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>      }
>  
>      if (virXPathNode("./group", ctxt) == NULL) {
> -        perms->gid = getgid();
> +        perms->gid = -1;
>      } else {
>          if (virXPathLong("number(./group)", ctxt, &v) < 0) {
>              virStorageReportError(VIR_ERR_XML_ERROR,
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 3742493..ee6a32e 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -241,11 +241,10 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
>      uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1;
>      gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1;
>      if (((uid != -1) || (gid != -1))
> -        && (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0)) {
> +        && (fchown(fd, uid, gid) < 0)) {
>          virReportSystemError(errno,
>                               _("cannot chown '%s' to (%u, %u)"),
> -                             vol->target.path, vol->target.perms.uid,
> -                             vol->target.perms.gid);
> +                             vol->target.path, uid, gid);
>          goto cleanup;
>      }
>      if (fchmod(fd, vol->target.perms.mode) < 0) {
> @@ -356,10 +355,12 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> +    uid_t uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
> +    gid_t gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;

This sounds good, but can we encapsulate this conditional logic in a helper
function or macro. eg

   uid_t uid = virVolumeDefGetUID(vol)

and so on for later areas of the patch


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]