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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list