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@xxxxxxxxxxxxx>
> ---
> 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list