[libvirt] [PATCH] Change default for storage uid/gid from getuid()/getgid() to -1/-1
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; + if ((createstat = virFileOperation(vol->target.path, O_RDWR | O_CREAT | O_EXCL | O_DSYNC, - vol->target.perms.mode, - vol->target.perms.uid, vol->target.perms.gid, + vol->target.perms.mode, uid, gid, createRawFileOpHook, &hdata, VIR_FILE_OP_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS @@ -491,14 +492,14 @@ cleanup: static int virStorageBuildSetUIDHook(void *data) { virStorageVolDefPtr vol = data; - if ((vol->target.perms.gid != 0) + if ((vol->target.perms.gid != -1) && (setgid(vol->target.perms.gid) != 0)) { virReportSystemError(errno, _("Cannot set gid to %u before creating %s"), vol->target.perms.gid, vol->target.path); return -1; } - if ((vol->target.perms.uid != 0) + if ((vol->target.perms.uid != -1) && (setuid(vol->target.perms.uid) != 0)) { virReportSystemError(errno, _("Cannot set uid to %u before creating %s"), @@ -517,8 +518,11 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, int filecreated = 0; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) - && (getuid() == 0) - && ((vol->target.perms.uid != 0) || (vol->target.perms.gid != 0))) { + && (((getuid() == 0) + && (vol->target.perms.uid != -1) + && (vol->target.perms.uid != 0)) + || ((vol->target.perms.gid != -1) + && (vol->target.perms.gid != getgid())))) { if (virRunWithHook(cmdargv, virStorageBuildSetUIDHook, vol, NULL) == 0) { /* command was successfully run, check if the file was created */ @@ -547,8 +551,7 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, && (chown(vol->target.path, 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); return -1; } if (chmod(vol->target.path, vol->target.perms.mode) < 0) { diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 8279d78..dfd417f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -529,16 +529,28 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, /* Now create the final dir in the path with the uid/gid/mode * requested in the config. If the dir already exists, just set * the perms. */ - if ((err = virDirCreate(pool->def->target.path, - pool->def->target.perms.mode, - pool->def->target.perms.uid, - pool->def->target.perms.gid, - VIR_DIR_CREATE_FORCE_PERMS | VIR_DIR_CREATE_ALLOW_EXIST | - (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) { - virReportSystemError(err, _("cannot create path '%s'"), - pool->def->target.path); - goto error; + + struct stat st; + + if ((stat(pool->def->target.path, &st) < 0) + || (pool->def->target.perms.uid != -1)) { + + uid_t uid = (pool->def->target.perms.uid == -1) + ? getuid() : pool->def->target.perms.uid; + gid_t gid = (pool->def->target.perms.gid == -1) + ? getgid() : pool->def->target.perms.gid; + + if ((err = virDirCreate(pool->def->target.path, + pool->def->target.perms.mode, + uid, gid, + VIR_DIR_CREATE_FORCE_PERMS | + VIR_DIR_CREATE_ALLOW_EXIST | + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) { + virReportSystemError(err, _("cannot create path '%s'"), + pool->def->target.path); + goto error; + } } ret = 0; error: @@ -777,8 +789,13 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } + 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; + if ((err = virDirCreate(vol->target.path, vol->target.perms.mode, - vol->target.perms.uid, vol->target.perms.gid, + uid, gid, VIR_DIR_CREATE_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS ? VIR_DIR_CREATE_AS_UID : 0))) != 0) { -- 1.6.6.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 :|
On 03/02/2010 05:48 AM, Daniel P. Berrange wrote:
On Tue, Mar 02, 2010 at 02:17:59AM -0500, Laine Stump wrote:
+ 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
Sure. That will be putting the getuid/getgid calls back into storage_conf (.h at least), but it will make things a bit more readable. I'll make a new version.
On 03/02/2010 09:51 AM, Laine Stump wrote:
On 03/02/2010 05:48 AM, Daniel P. Berrange wrote:
On Tue, Mar 02, 2010 at 02:17:59AM -0500, Laine Stump wrote:
+ 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
Sure. That will be putting the getuid/getgid calls back into storage_conf (.h at least), but it will make things a bit more readable.
I'll make a new version.
A few of us talked offline, and decided we should commit this patch as is for the release. Dan's proposed change is just a cleanup which can wait, and the patch fixes a regression when building pools with virt-manager, so I've just pushed it. Thanks, Cole
On 03/04/2010 05:54 PM, Cole Robinson wrote:
A few of us talked offline, and decided we should commit this patch as is for the release. Dan's proposed change is just a cleanup which can wait, and the patch fixes a regression when building pools with virt-manager, so I've just pushed it.
Thanks, Cole
Yes - Thanks, Cole!
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Laine Stump