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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list