Re: [libvirt] [PATCH 1/2] util: virFileOpenAs interprets -1 as "current uid|gid"
On 01/12/2012 12:33 PM, Laine Stump wrote:
[Oops. This is a prerequisite of the previous patch that I forgot to
send. That patch should be 2/2 and this should be 1/2.]
This just simplifies use of virFileOpenAs a bit - if you're in a place
where you don't have access to a different uid|gid, just give "-1".
---
src/libxl/libxl_driver.c | 4 ++--
src/storage/storage_backend.c | 8 +++-----
src/util/util.c | 4 ++++
3 files changed, 9 insertions(+), 7 deletions(-)
I like it; very much like chown()'s use of -1 to mean no change. Alas,
I have a nit:
POSIX permits uid_t to be an unsigned short. On implementations that
make that choice, casting (uid_t)-1 to an int gives 0xffff, rather than
-1, and comparison to -1 will fail. In general, it is safest if you use
an explicit cast when comparing to a uid_t or gid_t value in any
arithmetic operation, since the act of comparing might include an
implicit conversion to int.
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 0500ed0..d7325c3 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -216,7 +216,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
libxlSavefileHeader hdr;
char *xml = NULL;
- if ((fd = virFileOpenAs(from, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) {
+ if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
This use is fine, since virFileOpenAs is explicitly typed as taking
uid_t and gid_t: -1 is always correctly promoted to uid_t or gid_t.
+++ b/src/storage/storage_backend.c
@@ -380,8 +380,6 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
{
int ret = -1;
int fd = -1;
- uid_t uid;
- gid_t gid;
int operation_flags;
virCheckFlags(0, -1);
@@ -393,15 +391,15 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
goto cleanup;
}
- uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
- gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
Here, perms.uid is an int (hmm, I wonder if we should fix that in
virStoragePerms in conf/storage_conf.h, since that fails on platforms
where uid_t is larger than an int - but that's a fix for another day).
+++ b/src/util/util.c
@@ -848,6 +848,10 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
int pair[2] = { -1, -1 };
int forkRet;
+ /* allow using -1 to mean "current value" */
+ uid = (uid == -1) ? getuid() : uid;
+ gid = (gid == -1) ? getgid() : gid;
But here, you have a bug - the use of == does type promotion to the
common type no smaller than int, which means you need an explicit cast:
uid = (uid == (uid_t) -1) ? getuid() : uid;
And that's repetitive enough that I'd probably write it:
if (uid == (uid_t) -1)
uid = getuid();
Same for gid.
ACK with the 2 lines in util.c fixed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)