3:48 p.m.
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
3:48 p.m.
New subject: [libvirt] [PATCH 1/6] storage: fs: Don't overwrite virDirCreate error
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
3:36 a.m.
New subject: [libvirt] [PATCH 1/6] storage: fs: Don't overwrite virDirCreate error
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
3:48 p.m.
New subject: [libvirt] [PATCH 2/6] storage: fs: Fill in permissions on pool refresh
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
7:06 a.m.
New subject: [libvirt] [PATCH 2/6] storage: fs: Fill in permissions on pool refresh
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
8:39 p.m.
New subject: [libvirt] [PATCH 2/6] storage: fs: Fill in permissions on pool refresh
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
3:48 p.m.
New subject: [libvirt] [PATCH 3/6] storage: fs: Don't attempt directory creation if it already exists
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
7:19 a.m.
New subject: [libvirt] [PATCH 3/6] storage: fs: Don't attempt directory creation if it already exists
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
8:46 p.m.
New subject: [libvirt] [PATCH 3/6] storage: fs: Don't attempt directory creation if it already exists
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
1:02 a.m.
New subject: [libvirt] [PATCH 3/6] storage: fs: Don't attempt directory creation if it already exists
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) {
6:39 a.m.
New subject: [libvirt] [PATCH 3/6] storage: fs: Don't attempt directory creation if it already exists
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
10:36 p.m.
New subject: [libvirt] [PATCH 3/6] storage: fs: Don't attempt directory creation if it already exists
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
3:48 p.m.
New subject: [libvirt] [PATCH 4/6] storage: fs: Don't try to chown directory unless user requested
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
8:33 a.m.
New subject: [libvirt] [PATCH 4/6] storage: fs: Don't try to chown directory unless user requested
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
3:48 p.m.
New subject: [libvirt] [PATCH 5/6] storage: conf: Don't set any default <mode> in the XML
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
8:23 a.m.
New subject: [libvirt] [PATCH 5/6] storage: conf: Don't set any default <mode> in the XML
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
8:47 p.m.
New subject: [libvirt] [PATCH 5/6] storage: conf: Don't set any default <mode> in the XML
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
3:48 p.m.
New subject: [libvirt] [PATCH 6/6] storage: fs: Only force directory permissions if required
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
8:36 a.m.
New subject: [libvirt] [PATCH 6/6] storage: fs: Only force directory permissions if required
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
8:52 p.m.
New subject: [libvirt] [PATCH 6/6] storage: fs: Only force directory permissions if required
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
8:54 p.m.
New subject: [libvirt] [PATCH 6/6] storage: fs: Only force directory permissions if required
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
12:01 p.m.
New subject: [libvirt] [PATCH 6/6] storage: fs: Only force directory permissions if required
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
3446
days inactive
3527
days old
21 comments
4 participants
participants (4)
-
Christophe Fergeau -
Cole Robinson -
Peter Krempa -
tzheng