[libvirt] [PATCH 0/6] storage: fs: tweak dir perms handling on build
Consider the following issue - Using virt-manager with qemu:///session - User adds a storage pool pointing at /tmp. No explicit permissions are requested in the XML - virt-manager calls PoolDefine, then PoolBuild - libvirt tries to unconditionally chmod 755 /tmp. This fails because my user doesn't own root. Pool build fails, virt-manager reports failure Yes there's a couple ways we could avoid this specific case in virt-manager, but I think it makes more sense to have pool.build on a directory be a no-op in this case. The following patches address this. - Patch 1 is an error reporting tweak - Patch 2 is a feature, but implementing it simplifies later patches - Patch 3 makes pool.build not even attempt mkdir if the dir already exists. - Patch 4 makes pool.build skip dir chown'ing unless user explicitly requested uid or gid via the XML - Patch 5-6 make pool.build skip dir chmod unless the user explicitly requested <mode> via the XML. If a mode is required for mkdir, continue to use the previous default. Cole Robinson (6): storage: fs: Don't overwrite virDirCreate error storage: fs: Fill in permissions on pool refresh storage: fs: Don't attempt directory creation if it already exists storage: fs: Don't try to chown directory unless user requested storage: conf: Don't set any default <mode> in the XML storage: fs: Only force directory permissions if required docs/schemas/storagecommon.rng | 5 +- src/conf/storage_conf.c | 42 +++++------ src/storage/storage_backend.c | 20 ++++-- src/storage/storage_backend.h | 3 + src/storage/storage_backend_fs.c | 81 ++++++++++++++++------ src/storage/storage_backend_logical.c | 4 +- src/util/virfile.c | 47 ++++++++----- tests/storagepoolxml2xmlin/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 +- tests/storagevolxml2xmlin/vol-file.xml | 6 +- tests/storagevolxml2xmlout/vol-file.xml | 6 +- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 +- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 +- 14 files changed, 147 insertions(+), 77 deletions(-) -- 2.3.6
virDirCreate will give us fine grained details about what actually failed. --- src/storage/storage_backend_fs.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 521dc70..51d6bb3 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -813,8 +813,6 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, 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; } @@ -1055,8 +1053,6 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, VIR_DIR_CREATE_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS ? VIR_DIR_CREATE_AS_UID : 0))) < 0) { - virReportSystemError(-err, _("cannot create path '%s'"), - vol->target.path); return -1; } -- 2.3.6
On Mon, Apr 27, 2015 at 16:48:39 -0400, Cole Robinson wrote:
virDirCreate will give us fine grained details about what actually failed. --- src/storage/storage_backend_fs.c | 4 ---- 1 file changed, 4 deletions(-)
ACK, Peter
This means pool XML actually reports accurate user/group/mode/label. This uses UpdateVolTargetInfoFD in a bit of a hackish way, but it works --- src/storage/storage_backend_fs.c | 58 ++++++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 51d6bb3..804b7c3 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -816,12 +816,6 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; } - /* Reflect the actual uid and gid to the config. */ - if (pool->def->target.perms.uid == (uid_t) -1) - pool->def->target.perms.uid = geteuid(); - if (pool->def->target.perms.gid == (gid_t) -1) - pool->def->target.perms.gid = getegid(); - if (flags != 0) { ret = virStorageBackendMakeFileSystem(pool, flags); } else { @@ -845,8 +839,11 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, DIR *dir; struct dirent *ent; struct statvfs sb; + struct stat statbuf; virStorageVolDefPtr vol = NULL; + virStorageSourcePtr target = NULL; int direrr; + int fd = -1, ret = -1; if (!(dir = opendir(pool->def->target.path))) { virReportSystemError(errno, @@ -856,7 +853,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, } while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) { - int ret; + int err; if (virStringHasControlChars(ent->d_name)) { VIR_WARN("Ignoring file with control characters under '%s'", @@ -880,15 +877,15 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_STRDUP(vol->key, vol->target.path) < 0) goto error; - if ((ret = virStorageBackendProbeTarget(&vol->target, + if ((err = virStorageBackendProbeTarget(&vol->target, &vol->target.encryption)) < 0) { - if (ret == -2) { + if (err == -2) { /* Silently ignore non-regular files, * eg '.' '..', 'lost+found', dangling symbolic link */ virStorageVolDefFree(vol); vol = NULL; continue; - } else if (ret == -3) { + } else if (err == -3) { /* The backing file is currently unavailable, its format is not * explicitly specified, the probe to auto detect the format * failed: continue with faked RAW format, since AUTO will @@ -918,27 +915,60 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (direrr < 0) goto error; closedir(dir); + dir = NULL; + vol = NULL; + + if (VIR_ALLOC(target)) + goto error; + + if ((fd = open(pool->def->target.path, O_RDONLY)) < 0) { + virReportSystemError(errno, + _("cannot open path '%s'"), + pool->def->target.path); + goto error; + } + if (fstat(fd, &statbuf) < 0) { + virReportSystemError(errno, + _("cannot stat path '%s'"), + pool->def->target.path); + goto error; + } + + if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &statbuf) < 0) + goto error; + + /* VolTargetInfoFD doesn't update capacity correctly for the pool case */ if (statvfs(pool->def->target.path, &sb) < 0) { virReportSystemError(errno, _("cannot statvfs path '%s'"), pool->def->target.path); - return -1; + goto error; } + pool->def->capacity = ((unsigned long long)sb.f_frsize * (unsigned long long)sb.f_blocks); pool->def->available = ((unsigned long long)sb.f_bfree * (unsigned long long)sb.f_frsize); pool->def->allocation = pool->def->capacity - pool->def->available; - return 0; + pool->def->target.perms.mode = target->perms->mode; + pool->def->target.perms.uid = target->perms->uid; + pool->def->target.perms.gid = target->perms->gid; + VIR_FREE(pool->def->target.perms.label); + if (VIR_STRDUP(pool->def->target.perms.label, target->perms->label) < 0) + goto error; + ret = 0; error: if (dir) closedir(dir); + VIR_FORCE_CLOSE(fd); virStorageVolDefFree(vol); - virStoragePoolObjClearVols(pool); - return -1; + virStorageSourceFree(target); + if (ret < 0) + virStoragePoolObjClearVols(pool); + return ret; } -- 2.3.6
On Mon, Apr 27, 2015 at 16:48:40 -0400, Cole Robinson wrote:
This means pool XML actually reports accurate user/group/mode/label.
This uses UpdateVolTargetInfoFD in a bit of a hackish way, but it works --- src/storage/storage_backend_fs.c | 58 ++++++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 51d6bb3..804b7c3 100644 --- a/src/storage/storage_backend_fs.c
...
+ pool->def->target.perms.mode = target->perms->mode; + pool->def->target.perms.uid = target->perms->uid; + pool->def->target.perms.gid = target->perms->gid; + VIR_FREE(pool->def->target.perms.label); + if (VIR_STRDUP(pool->def->target.perms.label, target->perms->label) < 0) + goto error;
+ ret = 0; error:
Since both success and error paths use this label now, it should be called 'cleanup';
if (dir) closedir(dir); + VIR_FORCE_CLOSE(fd); virStorageVolDefFree(vol); - virStoragePoolObjClearVols(pool); - return -1; + virStorageSourceFree(target); + if (ret < 0) + virStoragePoolObjClearVols(pool); + return ret; }
ACK with the label renamed. Peter
On 04/28/2015 08:06 AM, Peter Krempa wrote:
On Mon, Apr 27, 2015 at 16:48:40 -0400, Cole Robinson wrote:
This means pool XML actually reports accurate user/group/mode/label.
This uses UpdateVolTargetInfoFD in a bit of a hackish way, but it works --- src/storage/storage_backend_fs.c | 58 ++++++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 51d6bb3..804b7c3 100644 --- a/src/storage/storage_backend_fs.c
...
+ pool->def->target.perms.mode = target->perms->mode; + pool->def->target.perms.uid = target->perms->uid; + pool->def->target.perms.gid = target->perms->gid; + VIR_FREE(pool->def->target.perms.label); + if (VIR_STRDUP(pool->def->target.perms.label, target->perms->label) < 0) + goto error;
+ ret = 0; error:
Since both success and error paths use this label now, it should be called 'cleanup';
if (dir) closedir(dir); + VIR_FORCE_CLOSE(fd); virStorageVolDefFree(vol); - virStoragePoolObjClearVols(pool); - return -1; + virStorageSourceFree(target); + if (ret < 0) + virStoragePoolObjClearVols(pool); + return ret; }
ACK with the label renamed.
Peter
Thanks, applied that change locally. Will push after the release is out - Cole
The current code attempts to handle this, but it only catches mkdir failing with EEXIST. However if say trying to build /tmp for an unprivileged qemu:///session, mkdir will fail with EPERM. Rather than catch any errors, just don't attempt mkdir if the directory already exists. --- src/util/virfile.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 87d121d..23a1655 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2289,12 +2289,13 @@ virDirCreateNoFork(const char *path, int ret = 0; struct stat st; - if ((mkdir(path, mode) < 0) - && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { - ret = -errno; - virReportSystemError(errno, _("failed to create directory '%s'"), - path); - goto error; + if (!(flags & VIR_DIR_CREATE_ALLOW_EXIST) || !virFileExists(path)) { + if (mkdir(path, mode) < 0) { + ret = -errno; + virReportSystemError(errno, _("failed to create directory '%s'"), + path); + goto error; + } } if (stat(path, &st) == -1) { -- 2.3.6
On Mon, Apr 27, 2015 at 16:48:41 -0400, Cole Robinson wrote:
The current code attempts to handle this, but it only catches mkdir failing with EEXIST. However if say trying to build /tmp for an unprivileged qemu:///session, mkdir will fail with EPERM.
Rather than catch any errors, just don't attempt mkdir if the directory already exists. --- src/util/virfile.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 87d121d..23a1655 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2289,12 +2289,13 @@ virDirCreateNoFork(const char *path, int ret = 0; struct stat st;
- if ((mkdir(path, mode) < 0) - && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { - ret = -errno; - virReportSystemError(errno, _("failed to create directory '%s'"), - path); - goto error; + if (!(flags & VIR_DIR_CREATE_ALLOW_EXIST) || !virFileExists(path)) {
De Morgan's law allows to optimize this into a more readable form: if (!(virFileExists(path) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { It's one set of parentheses more but it's readable.
+ if (mkdir(path, mode) < 0) { + ret = -errno; + virReportSystemError(errno, _("failed to create directory '%s'"), + path); + goto error; + }
ACK with the condition made human readable. Peter
On 04/28/2015 08:19 AM, Peter Krempa wrote:
On Mon, Apr 27, 2015 at 16:48:41 -0400, Cole Robinson wrote:
The current code attempts to handle this, but it only catches mkdir failing with EEXIST. However if say trying to build /tmp for an unprivileged qemu:///session, mkdir will fail with EPERM.
Rather than catch any errors, just don't attempt mkdir if the directory already exists. --- src/util/virfile.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 87d121d..23a1655 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2289,12 +2289,13 @@ virDirCreateNoFork(const char *path, int ret = 0; struct stat st;
- if ((mkdir(path, mode) < 0) - && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { - ret = -errno; - virReportSystemError(errno, _("failed to create directory '%s'"), - path); - goto error; + if (!(flags & VIR_DIR_CREATE_ALLOW_EXIST) || !virFileExists(path)) {
De Morgan's law allows to optimize this into a more readable form: if (!(virFileExists(path) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) {
It's one set of parentheses more but it's readable.
Huh, personally I find that form much less readable. You need to mentally evaluate the parenthesized expression, then remember to take the opposite. The first one I can read out loud from left to right: if not ALLOW_EXIST or not file exists: stuff Maybe I've just written too much python :) Regardless I've applied your suggestion locally, will push after the release. Thanks, Cole
+ if (mkdir(path, mode) < 0) { + ret = -errno; + virReportSystemError(errno, _("failed to create directory '%s'"), + path); + goto error; + }
ACK with the condition made human readable.
Peter
Hi,Cole After this patch is included in libvirt on rhel7.2,virt-manager can not create dir pool with existing directory. There is no default pool created in a fresh rhel7.2 system,so if I try to create default pool,the below error shows: Error creating pool: Could not build storage pool: failed to create directory '/var/lib/libvirt/images': File exists Do you think it's acceptable for this change since the directory(/var/lib/libvirt/images) is created by libvirt,if users want to create pool with /var/lib/libvirt/images,they need to delete images directory firstly,if so I think maybe some notes in doc is preferred,thanks very much. On 04/28/2015 04:48 AM, Cole Robinson wrote:
The current code attempts to handle this, but it only catches mkdir failing with EEXIST. However if say trying to build /tmp for an unprivileged qemu:///session, mkdir will fail with EPERM.
Rather than catch any errors, just don't attempt mkdir if the directory already exists. --- src/util/virfile.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 87d121d..23a1655 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2289,12 +2289,13 @@ virDirCreateNoFork(const char *path, int ret = 0; struct stat st;
- if ((mkdir(path, mode) < 0) - && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { - ret = -errno; - virReportSystemError(errno, _("failed to create directory '%s'"), - path); - goto error; + if (!(flags & VIR_DIR_CREATE_ALLOW_EXIST) || !virFileExists(path)) { + if (mkdir(path, mode) < 0) { + ret = -errno; + virReportSystemError(errno, _("failed to create directory '%s'"), + path); + goto error; + } }
if (stat(path, &st) == -1) {
On Thu, Jul 16, 2015 at 02:02:39PM +0800, tzheng wrote:
Hi,Cole
After this patch is included in libvirt on rhel7.2,virt-manager can not create dir pool with existing directory. There is no default pool created in a fresh rhel7.2 system,so if I try to create default pool,the below error shows: Error creating pool: Could not build storage pool: failed to create directory '/var/lib/libvirt/images': File exists
Do you think it's acceptable for this change since the directory(/var/lib/libvirt/images) is created by libvirt,if users want to create pool with /var/lib/libvirt/images,they need to delete images directory firstly,if so I think maybe some notes in doc is preferred,thanks very much.
This part of the code was then changed by https://libvirt.org/git/?p=libvirt.git;a=commit;h=c8661a1a7ee8ef11010 Are you 100% sure it's this patch which causes the issue you describe? GNOME Boxes is having very similar issues, see https://bugzilla.gnome.org/show_bug.cgi?id=752417 Christophe
On 07/16/2015 07:39 PM, Christophe Fergeau wrote:
On Thu, Jul 16, 2015 at 02:02:39PM +0800, tzheng wrote:
Hi,Cole
After this patch is included in libvirt on rhel7.2,virt-manager can not create dir pool with existing directory. There is no default pool created in a fresh rhel7.2 system,so if I try to create default pool,the below error shows: Error creating pool: Could not build storage pool: failed to create directory '/var/lib/libvirt/images': File exists
Do you think it's acceptable for this change since the directory(/var/lib/libvirt/images) is created by libvirt,if users want to create pool with /var/lib/libvirt/images,they need to delete images directory firstly,if so I think maybe some notes in doc is preferred,thanks very much. This part of the code was then changed by https://libvirt.org/git/?p=libvirt.git;a=commit;h=c8661a1a7ee8ef11010 Are you 100% sure it's this patch which causes the issue you describe?
I just searched this patch which is related to the change for build pool with existing directory.
GNOME Boxes is having very similar issues, see https://bugzilla.gnome.org/show_bug.cgi?id=752417
Thanks,it's the similar issue,I file a rhel7 bug:https://bugzilla.redhat.com/show_bug.cgi?id=1244080
Christophe
Currently we try to chown any directory passed to virDirCreate, even if the user didn't request any explicit owner/group via the pool/vol XML. This causes issues with qemu:///session: try to build a pool of a root owned directory like /tmp, and it fails trying to chown the directory to the session user. Instead it should just leave things as they are, unless the user requests changing permissions via the pool XML. Similarly this is annoying if creating a storage pool via system libvirtd of an existing directory in user $HOME, it's now owned by root. The virDirCreate function is pretty convoluted, since it needs to fork off in certain specific cases. Try to document that, to make it clear where exactly we are changing behavior. --- src/util/virfile.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 23a1655..676e7b4 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2303,7 +2303,8 @@ virDirCreateNoFork(const char *path, virReportSystemError(errno, _("stat of '%s' failed"), path); goto error; } - if (((st.st_uid != uid) || (st.st_gid != gid)) + if (((uid != (uid_t) -1 && st.st_uid != uid) || + (gid != (gid_t) -1 && st.st_gid != gid)) && (chown(path, uid, gid) < 0)) { ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), @@ -2335,19 +2336,32 @@ virDirCreate(const char *path, gid_t *groups; int ngroups; - /* allow using -1 to mean "current value" */ - if (uid == (uid_t) -1) - uid = geteuid(); - if (gid == (gid_t) -1) - gid = getegid(); - + /* Everything after this check is crazyness to allow setting uid/gid + * on directories that are on root-squash NFS shares. We only want + * to go that route if the follow conditions are true: + * + * 1) VIR_DIR_CREATE_AS_UID was passed, currently only used when + * directory is being created for a NETFS pool + * 2) We are running as root, since that's when the root-squash + * workaround is required. + * 3) An explicit uid/gid was requested + * 4) The directory doesn't already exist and the ALLOW_EXIST flag + * wasn't passed. + * + * If any of those conditions are _not_ met, ignore the fork crazyness + */ if ((!(flags & VIR_DIR_CREATE_AS_UID)) || (geteuid() != 0) - || ((uid == 0) && (gid == 0)) - || ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) { + || ((uid == (uid_t) -1) && (gid == (gid_t) -1)) + || ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && virFileExists(path))) { return virDirCreateNoFork(path, mode, uid, gid, flags); } + if (uid == (uid_t) -1) + uid = geteuid(); + if (gid == (gid_t) -1) + gid = getegid(); + ngroups = virGetGroupList(uid, gid, &groups); if (ngroups < 0) return -errno; -- 2.3.6
On Mon, Apr 27, 2015 at 16:48:42 -0400, Cole Robinson wrote:
Currently we try to chown any directory passed to virDirCreate, even if the user didn't request any explicit owner/group via the pool/vol XML.
This causes issues with qemu:///session: try to build a pool of a root owned directory like /tmp, and it fails trying to chown the directory to the session user. Instead it should just leave things as they are, unless the user requests changing permissions via the pool XML.
Similarly this is annoying if creating a storage pool via system libvirtd of an existing directory in user $HOME, it's now owned by root.
The virDirCreate function is pretty convoluted, since it needs to fork off in certain specific cases. Try to document that, to make it clear where exactly we are changing behavior. --- src/util/virfile.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
ACK, Peter
The XML parser sets a default <mode> if none is explicitly passed in. This is then used at pool/vol creation time, and unconditionally reported in the XML. The problem with this approach is that it's impossible for other code to determine if the user explicitly requested a storage mode. There are some cases where we want to make this distinction, but we currently can't. Handle <mode> parsing like we handle <owner>/<group>: if no value is passed in, set it to -1, and adjust the internal consumers to handle it. --- docs/schemas/storagecommon.rng | 5 ++- src/conf/storage_conf.c | 42 +++++++++++----------- src/storage/storage_backend.c | 20 ++++++++--- src/storage/storage_backend.h | 3 ++ src/storage/storage_backend_fs.c | 9 +++-- src/storage/storage_backend_logical.c | 4 ++- tests/storagepoolxml2xmlin/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 +- tests/storagevolxml2xmlin/vol-file.xml | 6 ++-- tests/storagevolxml2xmlout/vol-file.xml | 6 ++-- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 +- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 +- 13 files changed, 64 insertions(+), 41 deletions(-) diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 5f71b10..e4e8a51 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -99,7 +99,10 @@ <element name='permissions'> <interleave> <element name='mode'> - <ref name='octalMode'/> + <choice> + <ref name='octalMode'/> + <value>-1</value> + </choice> </element> <element name='owner'> <choice> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4852dfb..7131242 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -50,9 +50,6 @@ VIR_LOG_INIT("conf.storage_conf"); -#define DEFAULT_POOL_PERM_MODE 0755 -#define DEFAULT_VOL_PERM_MODE 0600 - VIR_ENUM_IMPL(virStorageVol, VIR_STORAGE_VOL_LAST, "file", "block", "dir", "network", "netdir") @@ -718,8 +715,7 @@ virStoragePoolDefParseSourceString(const char *srcSpec, static int virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePermsPtr perms, - const char *permxpath, - int defaultmode) + const char *permxpath) { char *mode; long long val; @@ -730,7 +726,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, node = virXPathNode(permxpath, ctxt); if (node == NULL) { /* Set default values if there is not <permissions> element */ - perms->mode = defaultmode; + perms->mode = (mode_t) -1; perms->uid = (uid_t) -1; perms->gid = (gid_t) -1; perms->label = NULL; @@ -740,13 +736,12 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, relnode = ctxt->node; ctxt->node = node; - mode = virXPathString("string(./mode)", ctxt); - if (!mode) { - perms->mode = defaultmode; - } else { + if ((mode = virXPathString("string(./mode)", ctxt))) { int tmp; - if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || + ((tmp & ~0777) && + tmp != -1)) { VIR_FREE(mode); virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed octal mode")); @@ -754,6 +749,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } perms->mode = tmp; VIR_FREE(mode); + } else { + perms->mode = (mode_t) -1; } if (virXPathNode("./owner", ctxt) == NULL) { @@ -947,8 +944,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; if (virStorageDefParsePerms(ctxt, &ret->target.perms, - "./target/permissions", - DEFAULT_POOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error; } @@ -1185,8 +1181,11 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->target.perms.mode); + if (def->target.perms.mode == (mode_t) -1) + virBufferAddLit(buf, "<mode>-1</mode>\n"); + else + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->target.perms.mode); virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->target.perms.uid); virBufferAsprintf(buf, "<group>%d</group>\n", @@ -1315,8 +1314,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(ret->target.backingStore->perms) < 0) goto error; if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms, - "./backingStore/permissions", - DEFAULT_VOL_PERM_MODE) < 0) + "./backingStore/permissions") < 0) goto error; } @@ -1361,8 +1359,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, if (VIR_ALLOC(ret->target.perms) < 0) goto error; if (virStorageDefParsePerms(ctxt, ret->target.perms, - "./target/permissions", - DEFAULT_VOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error; node = virXPathNode("./target/encryption", ctxt); @@ -1520,8 +1517,11 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->perms->mode); + if (def->perms->mode == (mode_t) -1) + virBufferAddLit(buf, "<mode>-1</mode>\n"); + else + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->perms->mode); virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->perms->uid); virBufferAsprintf(buf, "<group>%d</group>\n", diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e0311e1..1bf79db 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -318,6 +318,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, struct stat st; gid_t gid; uid_t uid; + mode_t mode; bool reflink_copy = false; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | @@ -367,10 +368,13 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, (unsigned int) gid); goto cleanup; } - if (fchmod(fd, vol->target.perms->mode) < 0) { + + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + if (fchmod(fd, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms->mode); + vol->target.path, mode); goto cleanup; } if (VIR_CLOSE(fd) < 0) { @@ -509,7 +513,9 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms->mode, + (vol->target.perms->mode ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode), vol->target.perms->uid, vol->target.perms->gid, operation_flags)) < 0) { @@ -664,6 +670,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, struct stat st; gid_t gid; uid_t uid; + mode_t mode; bool filecreated = false; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -709,10 +716,13 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, (unsigned int) gid); return -1; } - if (chmod(vol->target.path, vol->target.perms->mode) < 0) { + + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + if (chmod(vol->target.path, mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms->mode); + vol->target.path, mode); return -1; } return 0; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 85a8a4b..39c00b1 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -177,6 +177,9 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755 +# define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600 + int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withBlockVolFormat, unsigned int openflags); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 804b7c3..344ee4c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -806,7 +806,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, * 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.mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_POOL_PERM_MODE : + pool->def->target.perms.mode), pool->def->target.perms.uid, pool->def->target.perms.gid, VIR_DIR_CREATE_FORCE_PERMS | @@ -1077,7 +1079,10 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, } - if ((err = virDirCreate(vol->target.path, vol->target.perms->mode, + if ((err = virDirCreate(vol->target.path, + (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode), vol->target.perms->uid, vol->target.perms->gid, VIR_DIR_CREATE_FORCE_PERMS | diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 11c5683..9c77b4c 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -787,7 +787,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, goto error; } } - if (fchmod(fd, vol->target.perms->mode) < 0) { + if (fchmod(fd, (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : + vol->target.perms->mode)) < 0) { virReportSystemError(errno, _("cannot set file mode '%s'"), vol->target.path); diff --git a/tests/storagepoolxml2xmlin/pool-dir.xml b/tests/storagepoolxml2xmlin/pool-dir.xml index e10ccb7..6b30053 100644 --- a/tests/storagepoolxml2xmlin/pool-dir.xml +++ b/tests/storagepoolxml2xmlin/pool-dir.xml @@ -9,7 +9,7 @@ <target> <path>///var/////lib/libvirt/images//</path> <permissions> - <mode>0700</mode> + <mode>-1</mode> <owner>-1</owner> <group>-1</group> <label>some_label_t</label> diff --git a/tests/storagepoolxml2xmlout/pool-dir.xml b/tests/storagepoolxml2xmlout/pool-dir.xml index f81bc1d..9b926fe 100644 --- a/tests/storagepoolxml2xmlout/pool-dir.xml +++ b/tests/storagepoolxml2xmlout/pool-dir.xml @@ -9,7 +9,7 @@ <target> <path>/var/lib/libvirt/images</path> <permissions> - <mode>0700</mode> + <mode>-1</mode> <owner>-1</owner> <group>-1</group> <label>some_label_t</label> diff --git a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml index bab2a15..d91fe03 100644 --- a/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml +++ b/tests/storagepoolxml2xmlout/pool-netfs-gluster.xml @@ -12,7 +12,7 @@ <target> <path>/mnt/gluster</path> <permissions> - <mode>0755</mode> + <mode>-1</mode> <owner>-1</owner> <group>-1</group> </permissions> diff --git a/tests/storagevolxml2xmlin/vol-file.xml b/tests/storagevolxml2xmlin/vol-file.xml index d3f65f6..ffb472e 100644 --- a/tests/storagevolxml2xmlin/vol-file.xml +++ b/tests/storagevolxml2xmlin/vol-file.xml @@ -6,9 +6,9 @@ <target> <path>/var/lib/libvirt/images/sparse.img</path> <permissions> - <mode>0</mode> - <owner>0744</owner> - <group>0</group> + <mode>-1</mode> + <owner>-1</owner> + <group>-1</group> <label>virt_image_t</label> </permissions> <timestamps> diff --git a/tests/storagevolxml2xmlout/vol-file.xml b/tests/storagevolxml2xmlout/vol-file.xml index 2923188..aa9ac6b 100644 --- a/tests/storagevolxml2xmlout/vol-file.xml +++ b/tests/storagevolxml2xmlout/vol-file.xml @@ -8,9 +8,9 @@ <path>/var/lib/libvirt/images/sparse.img</path> <format type='raw'/> <permissions> - <mode>00</mode> - <owner>744</owner> - <group>0</group> + <mode>-1</mode> + <owner>-1</owner> + <group>-1</group> <label>virt_image_t</label> </permissions> </target> diff --git a/tests/storagevolxml2xmlout/vol-gluster-dir.xml b/tests/storagevolxml2xmlout/vol-gluster-dir.xml index 538b31d..067be0f 100644 --- a/tests/storagevolxml2xmlout/vol-gluster-dir.xml +++ b/tests/storagevolxml2xmlout/vol-gluster-dir.xml @@ -9,7 +9,7 @@ <path>gluster://example.com/vol/dir</path> <format type='dir'/> <permissions> - <mode>0600</mode> + <mode>-1</mode> <owner>-1</owner> <group>-1</group> </permissions> diff --git a/tests/storagevolxml2xmlout/vol-sheepdog.xml b/tests/storagevolxml2xmlout/vol-sheepdog.xml index 0a1f32c..6b8355f 100644 --- a/tests/storagevolxml2xmlout/vol-sheepdog.xml +++ b/tests/storagevolxml2xmlout/vol-sheepdog.xml @@ -8,7 +8,7 @@ <path>sheepdog:test2</path> <format type='unknown'/> <permissions> - <mode>0600</mode> + <mode>-1</mode> <owner>-1</owner> <group>-1</group> </permissions> -- 2.3.6
On Mon, Apr 27, 2015 at 16:48:43 -0400, Cole Robinson wrote:
The XML parser sets a default <mode> if none is explicitly passed in. This is then used at pool/vol creation time, and unconditionally reported in the XML.
The problem with this approach is that it's impossible for other code to determine if the user explicitly requested a storage mode. There are some cases where we want to make this distinction, but we currently can't.
Handle <mode> parsing like we handle <owner>/<group>: if no value is passed in, set it to -1, and adjust the internal consumers to handle it. --- docs/schemas/storagecommon.rng | 5 ++- src/conf/storage_conf.c | 42 +++++++++++----------- src/storage/storage_backend.c | 20 ++++++++--- src/storage/storage_backend.h | 3 ++ src/storage/storage_backend_fs.c | 9 +++-- src/storage/storage_backend_logical.c | 4 ++- tests/storagepoolxml2xmlin/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 +- tests/storagevolxml2xmlin/vol-file.xml | 6 ++-- tests/storagevolxml2xmlout/vol-file.xml | 6 ++-- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 +- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 +- 13 files changed, 64 insertions(+), 41 deletions(-)
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 5f71b10..e4e8a51 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -99,7 +99,10 @@ <element name='permissions'> <interleave> <element name='mode'> - <ref name='octalMode'/> + <choice> + <ref name='octalMode'/> + <value>-1</value> + </choice>
I'd rather make the mode optional if you want to keep the default value.
</element> <element name='owner'> <choice> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4852dfb..7131242 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -50,9 +50,6 @@
VIR_LOG_INIT("conf.storage_conf");
-#define DEFAULT_POOL_PERM_MODE 0755 -#define DEFAULT_VOL_PERM_MODE 0600 - VIR_ENUM_IMPL(virStorageVol, VIR_STORAGE_VOL_LAST, "file", "block", "dir", "network", "netdir") @@ -718,8 +715,7 @@ virStoragePoolDefParseSourceString(const char *srcSpec, static int virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePermsPtr perms, - const char *permxpath, - int defaultmode) + const char *permxpath) { char *mode; long long val; @@ -730,7 +726,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, node = virXPathNode(permxpath, ctxt); if (node == NULL) { /* Set default values if there is not <permissions> element */ - perms->mode = defaultmode; + perms->mode = (mode_t) -1; perms->uid = (uid_t) -1; perms->gid = (gid_t) -1; perms->label = NULL; @@ -740,13 +736,12 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, relnode = ctxt->node; ctxt->node = node;
- mode = virXPathString("string(./mode)", ctxt); - if (!mode) { - perms->mode = defaultmode; - } else { + if ((mode = virXPathString("string(./mode)", ctxt))) { int tmp;
- if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || + ((tmp & ~0777) && + tmp != -1)) { VIR_FREE(mode); virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed octal mode")); @@ -754,6 +749,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } perms->mode = tmp; VIR_FREE(mode); + } else { + perms->mode = (mode_t) -1; }
if (virXPathNode("./owner", ctxt) == NULL) { @@ -947,8 +944,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error;
if (virStorageDefParsePerms(ctxt, &ret->target.perms, - "./target/permissions", - DEFAULT_POOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error; }
@@ -1185,8 +1181,11 @@ virStoragePoolDefFormatBuf(virBufferPtr buf,
virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->target.perms.mode); + if (def->target.perms.mode == (mode_t) -1) + virBufferAddLit(buf, "<mode>-1</mode>\n");
And I'd skip formatting it.
+ else + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->target.perms.mode); virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->target.perms.uid); virBufferAsprintf(buf, "<group>%d</group>\n",
Using -1 looks rather ugly. Peter
On 04/28/2015 09:23 AM, Peter Krempa wrote:
On Mon, Apr 27, 2015 at 16:48:43 -0400, Cole Robinson wrote:
The XML parser sets a default <mode> if none is explicitly passed in. This is then used at pool/vol creation time, and unconditionally reported in the XML.
The problem with this approach is that it's impossible for other code to determine if the user explicitly requested a storage mode. There are some cases where we want to make this distinction, but we currently can't.
Handle <mode> parsing like we handle <owner>/<group>: if no value is passed in, set it to -1, and adjust the internal consumers to handle it. --- docs/schemas/storagecommon.rng | 5 ++- src/conf/storage_conf.c | 42 +++++++++++----------- src/storage/storage_backend.c | 20 ++++++++--- src/storage/storage_backend.h | 3 ++ src/storage/storage_backend_fs.c | 9 +++-- src/storage/storage_backend_logical.c | 4 ++- tests/storagepoolxml2xmlin/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-dir.xml | 2 +- tests/storagepoolxml2xmlout/pool-netfs-gluster.xml | 2 +- tests/storagevolxml2xmlin/vol-file.xml | 6 ++-- tests/storagevolxml2xmlout/vol-file.xml | 6 ++-- tests/storagevolxml2xmlout/vol-gluster-dir.xml | 2 +- tests/storagevolxml2xmlout/vol-sheepdog.xml | 2 +- 13 files changed, 64 insertions(+), 41 deletions(-)
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 5f71b10..e4e8a51 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -99,7 +99,10 @@ <element name='permissions'> <interleave> <element name='mode'> - <ref name='octalMode'/> + <choice> + <ref name='octalMode'/> + <value>-1</value> + </choice>
I'd rather make the mode optional if you want to keep the default value.
</element> <element name='owner'> <choice> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4852dfb..7131242 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -50,9 +50,6 @@
VIR_LOG_INIT("conf.storage_conf");
-#define DEFAULT_POOL_PERM_MODE 0755 -#define DEFAULT_VOL_PERM_MODE 0600 - VIR_ENUM_IMPL(virStorageVol, VIR_STORAGE_VOL_LAST, "file", "block", "dir", "network", "netdir") @@ -718,8 +715,7 @@ virStoragePoolDefParseSourceString(const char *srcSpec, static int virStorageDefParsePerms(xmlXPathContextPtr ctxt, virStoragePermsPtr perms, - const char *permxpath, - int defaultmode) + const char *permxpath) { char *mode; long long val; @@ -730,7 +726,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, node = virXPathNode(permxpath, ctxt); if (node == NULL) { /* Set default values if there is not <permissions> element */ - perms->mode = defaultmode; + perms->mode = (mode_t) -1; perms->uid = (uid_t) -1; perms->gid = (gid_t) -1; perms->label = NULL; @@ -740,13 +736,12 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, relnode = ctxt->node; ctxt->node = node;
- mode = virXPathString("string(./mode)", ctxt); - if (!mode) { - perms->mode = defaultmode; - } else { + if ((mode = virXPathString("string(./mode)", ctxt))) { int tmp;
- if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) { + if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || + ((tmp & ~0777) && + tmp != -1)) { VIR_FREE(mode); virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed octal mode")); @@ -754,6 +749,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } perms->mode = tmp; VIR_FREE(mode); + } else { + perms->mode = (mode_t) -1; }
if (virXPathNode("./owner", ctxt) == NULL) { @@ -947,8 +944,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error;
if (virStorageDefParsePerms(ctxt, &ret->target.perms, - "./target/permissions", - DEFAULT_POOL_PERM_MODE) < 0) + "./target/permissions") < 0) goto error; }
@@ -1185,8 +1181,11 @@ virStoragePoolDefFormatBuf(virBufferPtr buf,
virBufferAddLit(buf, "<permissions>\n"); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->target.perms.mode); + if (def->target.perms.mode == (mode_t) -1) + virBufferAddLit(buf, "<mode>-1</mode>\n");
And I'd skip formatting it.
+ else + virBufferAsprintf(buf, "<mode>0%o</mode>\n", + def->target.perms.mode); virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->target.perms.uid); virBufferAsprintf(buf, "<group>%d</group>\n",
Using -1 looks rather ugly.
Agreed, but I wanted to keep parity with how we handle uid/gid, they will output -1 as well. After the release I'll come back to this and add an extra patch to drop uid/gid outputing -1, then alter this patch to match Thanks, Cole
Only set directory permissions at pool build time, if: - User explicitly requested a mode via the XML - The directory needs to be created - We need to do the crazy NFS root-squash workaround This allows qemu:///session to call build on an existing directory like /tmp. --- src/storage/storage_backend_fs.c | 16 +++++++++++----- src/util/virfile.c | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 344ee4c..e11c240 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -769,6 +769,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, int err, ret = -1; char *parent = NULL; char *p = NULL; + mode_t mode; + bool needs_create_as_uid; virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); @@ -802,19 +804,23 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, } } + needs_create_as_uid = (pool->def->type == VIR_STORAGE_POOL_NETFS); + mode = pool->def->target.perms.mode; + if (mode == (mode_t) -1 && + (needs_create_as_uid || !virFileExists(pool->def->target.path))) + mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE; + /* 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 == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_POOL_PERM_MODE : - pool->def->target.perms.mode), + 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) { + (needs_create_as_uid ? VIR_DIR_CREATE_AS_UID : 0) + )) < 0) { goto error; } diff --git a/src/util/virfile.c b/src/util/virfile.c index 676e7b4..7e49f39 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,7 +2311,7 @@ virDirCreateNoFork(const char *path, path, (unsigned int) uid, (unsigned int) gid); goto error; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + if (((flags & VIR_DIR_CREATE_FORCE_PERMS) && mode != (mode_t) -1) && (chmod(path, mode) < 0)) { ret = -errno; virReportSystemError(errno, -- 2.3.6
On Mon, Apr 27, 2015 at 16:48:44 -0400, Cole Robinson wrote:
Only set directory permissions at pool build time, if:
- User explicitly requested a mode via the XML - The directory needs to be created - We need to do the crazy NFS root-squash workaround
This allows qemu:///session to call build on an existing directory like /tmp. --- src/storage/storage_backend_fs.c | 16 +++++++++++----- src/util/virfile.c | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 344ee4c..e11c240 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -769,6 +769,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, int err, ret = -1; char *parent = NULL; char *p = NULL; + mode_t mode; + bool needs_create_as_uid;
virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); @@ -802,19 +804,23 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ needs_create_as_uid = (pool->def->type == VIR_STORAGE_POOL_NETFS); + mode = pool->def->target.perms.mode; + if (mode == (mode_t) -1 && + (needs_create_as_uid || !virFileExists(pool->def->target.path))) + mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE; + /* 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 == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_POOL_PERM_MODE : - pool->def->target.perms.mode), + 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) { + (needs_create_as_uid ? VIR_DIR_CREATE_AS_UID : 0) + )) < 0) {
Closing parentheses on a separate line are ugly. I'd rather see violating the 80 cols rule rather than damaging readability of the code.
goto error; }
diff --git a/src/util/virfile.c b/src/util/virfile.c index 676e7b4..7e49f39 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,7 +2311,7 @@ virDirCreateNoFork(const char *path, path, (unsigned int) uid, (unsigned int) gid); goto error; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + if (((flags & VIR_DIR_CREATE_FORCE_PERMS) && mode != (mode_t) -1)
This is a usage error of the function. Using mode == -1 and requesting it to be set doesn't make much sense.
&& (chmod(path, mode) < 0)) { ret = -errno; virReportSystemError(errno,
ACK. Peter
On 04/28/2015 09:36 AM, Peter Krempa wrote:
On Mon, Apr 27, 2015 at 16:48:44 -0400, Cole Robinson wrote:
Only set directory permissions at pool build time, if:
- User explicitly requested a mode via the XML - The directory needs to be created - We need to do the crazy NFS root-squash workaround
This allows qemu:///session to call build on an existing directory like /tmp. --- src/storage/storage_backend_fs.c | 16 +++++++++++----- src/util/virfile.c | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 344ee4c..e11c240 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -769,6 +769,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, int err, ret = -1; char *parent = NULL; char *p = NULL; + mode_t mode; + bool needs_create_as_uid;
virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); @@ -802,19 +804,23 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ needs_create_as_uid = (pool->def->type == VIR_STORAGE_POOL_NETFS); + mode = pool->def->target.perms.mode; + if (mode == (mode_t) -1 && + (needs_create_as_uid || !virFileExists(pool->def->target.path))) + mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE; + /* 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 == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_POOL_PERM_MODE : - pool->def->target.perms.mode), + 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) { + (needs_create_as_uid ? VIR_DIR_CREATE_AS_UID : 0) + )) < 0) {
Closing parentheses on a separate line are ugly. I'd rather see violating the 80 cols rule rather than damaging readability of the code.
Will do
goto error; }
diff --git a/src/util/virfile.c b/src/util/virfile.c index 676e7b4..7e49f39 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,7 +2311,7 @@ virDirCreateNoFork(const char *path, path, (unsigned int) uid, (unsigned int) gid); goto error; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + if (((flags & VIR_DIR_CREATE_FORCE_PERMS) && mode != (mode_t) -1)
This is a usage error of the function. Using mode == -1 and requesting it to be set doesn't make much sense.
Hmm, I think instead I'll add an additional patch to drop FORCE_PERMS entirely.. it's used by every virDirCreate caller. We can just key the chmod off of whether mode != -1
&& (chmod(path, mode) < 0)) { ret = -errno; virReportSystemError(errno,
ACK.
Peter
On 04/28/2015 09:36 AM, Peter Krempa wrote:
On Mon, Apr 27, 2015 at 16:48:44 -0400, Cole Robinson wrote:
Only set directory permissions at pool build time, if:
- User explicitly requested a mode via the XML - The directory needs to be created - We need to do the crazy NFS root-squash workaround
This allows qemu:///session to call build on an existing directory like /tmp. --- src/storage/storage_backend_fs.c | 16 +++++++++++----- src/util/virfile.c | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 344ee4c..e11c240 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -769,6 +769,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, int err, ret = -1; char *parent = NULL; char *p = NULL; + mode_t mode; + bool needs_create_as_uid;
virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); @@ -802,19 +804,23 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ needs_create_as_uid = (pool->def->type == VIR_STORAGE_POOL_NETFS); + mode = pool->def->target.perms.mode; + if (mode == (mode_t) -1 && + (needs_create_as_uid || !virFileExists(pool->def->target.path))) + mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE; + /* 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 == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_POOL_PERM_MODE : - pool->def->target.perms.mode), + 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) { + (needs_create_as_uid ? VIR_DIR_CREATE_AS_UID : 0) + )) < 0) {
Closing parentheses on a separate line are ugly. I'd rather see violating the 80 cols rule rather than damaging readability of the code.
goto error; }
diff --git a/src/util/virfile.c b/src/util/virfile.c index 676e7b4..7e49f39 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,7 +2311,7 @@ virDirCreateNoFork(const char *path, path, (unsigned int) uid, (unsigned int) gid); goto error; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + if (((flags & VIR_DIR_CREATE_FORCE_PERMS) && mode != (mode_t) -1)
This is a usage error of the function. Using mode == -1 and requesting it to be set doesn't make much sense.
And to clarify, I'll push patches 1-4 when the new release opens, since they had minor changes. Then I'll post a new series with 5-6 + additional patches - Cole
On 04/28/2015 09:54 PM, Cole Robinson wrote:
On 04/28/2015 09:36 AM, Peter Krempa wrote:
On Mon, Apr 27, 2015 at 16:48:44 -0400, Cole Robinson wrote:
Only set directory permissions at pool build time, if:
- User explicitly requested a mode via the XML - The directory needs to be created - We need to do the crazy NFS root-squash workaround
This allows qemu:///session to call build on an existing directory like /tmp. --- src/storage/storage_backend_fs.c | 16 +++++++++++----- src/util/virfile.c | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 344ee4c..e11c240 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -769,6 +769,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, int err, ret = -1; char *parent = NULL; char *p = NULL; + mode_t mode; + bool needs_create_as_uid;
virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); @@ -802,19 +804,23 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ needs_create_as_uid = (pool->def->type == VIR_STORAGE_POOL_NETFS); + mode = pool->def->target.perms.mode; + if (mode == (mode_t) -1 && + (needs_create_as_uid || !virFileExists(pool->def->target.path))) + mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE; + /* 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 == (mode_t) -1 ? - VIR_STORAGE_DEFAULT_POOL_PERM_MODE : - pool->def->target.perms.mode), + 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) { + (needs_create_as_uid ? VIR_DIR_CREATE_AS_UID : 0) + )) < 0) {
Closing parentheses on a separate line are ugly. I'd rather see violating the 80 cols rule rather than damaging readability of the code.
goto error; }
diff --git a/src/util/virfile.c b/src/util/virfile.c index 676e7b4..7e49f39 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2311,7 +2311,7 @@ virDirCreateNoFork(const char *path, path, (unsigned int) uid, (unsigned int) gid); goto error; } - if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + if (((flags & VIR_DIR_CREATE_FORCE_PERMS) && mode != (mode_t) -1)
This is a usage error of the function. Using mode == -1 and requesting it to be set doesn't make much sense.
And to clarify, I'll push patches 1-4 when the new release opens, since they had minor changes. Then I'll post a new series with 5-6 + additional patches
Pushed patches 1-4 now. Thanks, Cole
participants (4)
-
Christophe Fergeau -
Cole Robinson -
Peter Krempa -
tzheng