[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] storage: fix uid_t|gid_t handling on 32 bit Linux



On 02/22/2013 09:55 AM, Philipp Hahn wrote:
> uid_t and gid_t are opaque types, ranging from s32 to u32 to u64.
> 
> Unfortunately libvirt uses the value -1 to mean "current process", which
> on Linux32 gets converted to ALLONE := +(2^32-1) = 4294967295.

I like the standardized name INT_MAX better than the invented name ALLONE.

> 
> Different libvirt versions used different formatting in the past, which
> break one or the other parsing function:
> virXPathLong(): (signed long)-1 != (double)ALLONE
> virXPathULong(): (unsigned long)-1 != (double)-1
> 
> Roll our own version of virXPathULong(), which also accepts -1, which is
> silently converted to ALLONE.
> 
> For output: do the reverse and print -1 instead of ALLONE.
> 
> Signed-off-by: Philipp Hahn <hahn univention de>
> ---
>  src/conf/storage_conf.c |   74 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 60 insertions(+), 14 deletions(-)
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 9134a22..b267f00 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -665,7 +665,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>                          const char *permxpath,
>                          int defaultmode) {
>      char *mode;
> -    long v;
> +    double d;

Eww - why do we need to use a double?  Just parse to a 32-bit int, then
check for truncation after the fact (as I said elsewhere in the series,
we already do a compile-time verify that uid_t is not larger than int;
so the real problem is if uid_t is smaller than int).

>      int ret = -1;
>      xmlNodePtr relnode;
>      xmlNodePtr node;
> @@ -699,26 +699,57 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
>          VIR_FREE(mode);
>      }
>  
> +    /* uid_t and gid_t are *opaque* types: on Linux they're u32, on Windows64
> +     * they're u64, on Solaris they were s32 in the past. There may be others.

Not accurate.  mingw64 has s64 pid_t (not u64); and completely lacks
uid_t and gid_t natively:

$ printf '#include <sys/types.h>\nuid_t\n' | \
	x86_64-w64-mingw32-gcc -E -|grep id_t
typedef long long _pid_t;
typedef _pid_t pid_t;
uid_t

When using gnulib (as libvirt does), gnulib provides [ug]id_t as 32-bit
types even on mingw64.

Furthermore, a lot of the problems stem from the fact that s16 or u16
uid_t/gid_t have been used in the past (u16 promotes to s32 without sign
extension, so it does not compare equal to (s32)-1).

I like the idea of outputting -1 rather than the unsigned all-ones
value; which means you need to respin this patch to avoid testsuite
failures where we change the test output.  Also, anywhere that the
parser doesn't accept -1, we need to fix that; accepting -1 as the only
valid negative value and requiring all other values to be non-negative
makes sense.

Looking forward to v2; this patch is appropriate to get into the 1.0.3
release if we get it respun in time.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]