[libvirt] [PATCH 1/2] util: virFileOpenAs interprets -1 as "current uid|gid"

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120112/261c0dcb/attachment-0001.sig>