uid_t and gid_t are opaque types, ranging from s32 to u32 to u64. Explicitly cast the magic -1 to the appropriate type. Signed-off-by: Philipp Hahn <hahn(a)univention.de&gt; --- src/conf/storage_conf.c | 8 ++++---- src/storage/storage_backend.c | 16 ++++++++-------- src/util/virutil.c | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 7a39998..9134a22 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -674,8 +674,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, if (node == NULL) { /* Set default values if there is not <permissions> element */ perms->mode = defaultmode; - perms->uid = -1; - perms->gid = -1; + perms->uid = (uid_t) -1; + perms->gid = (gid_t) -1; perms->label = NULL; return 0; } @@ -700,7 +700,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } if (virXPathNode("./owner", ctxt) == NULL) { - perms->uid = -1; + perms->uid = (uid_t) -1; } else { if (virXPathLong("number(./owner)", ctxt, &v) < 0) { virReportError(VIR_ERR_XML_ERROR, @@ -711,7 +711,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt, } if (virXPathNode("./group", ctxt) == NULL) { - perms->gid = -1; + perms->gid = (gid_t) -1; } else { if (virXPathLong("number(./group)", ctxt, &v) < 0) { virReportError(VIR_ERR_XML_ERROR, diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 964ce29..ec2fa53 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -277,9 +277,9 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.path); goto cleanup; } - uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; - if (((uid != -1) || (gid != -1)) + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1; + gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1; + if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) && (fchown(fd, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), @@ -542,9 +542,9 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((getuid() == 0) - && (vol->target.perms.uid != -1) + && (vol->target.perms.uid != (uid_t) -1) && (vol->target.perms.uid != 0)) - || ((vol->target.perms.gid != -1) + || ((vol->target.perms.gid != (gid_t) -1) && (vol->target.perms.gid != getgid())))) { virCommandSetUID(cmd, vol->target.perms.uid); @@ -572,9 +572,9 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, } } - uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; - if (((uid != -1) || (gid != -1)) + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1; + gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1; + if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) && (chown(vol->target.path, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown %s to (%u, %u)"), diff --git a/src/util/virutil.c b/src/util/virutil.c index 563f684..4af2599 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1206,7 +1206,7 @@ parenterror: _("stat of '%s' failed"), path); goto childerror; } - if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + if ((st.st_gid != gid) && (chown(path, (uid_t) -1, gid) < 0)) { ret = -errno; virReportSystemError(errno, _("cannot chown '%s' to group %u"), -- 1.7.10.4

uid_t and gid_t are opaque types, ranging from s32 to u32 to u64. Explicitly cast them to unsigned int for printing. Signed-off-by: Philipp Hahn <hahn(a)univention.de&gt; --- src/storage/storage_backend.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 019b09a..964ce29 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -283,7 +283,8 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, && (fchown(fd, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), - vol->target.path, uid, gid); + vol->target.path, (unsigned int) uid, + (unsigned int) gid); goto cleanup; } if (fchmod(fd, vol->target.perms.mode) < 0) { @@ -577,7 +578,8 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, && (chown(vol->target.path, uid, gid) < 0)) { virReportSystemError(errno, _("cannot chown %s to (%u, %u)"), - vol->target.path, uid, gid); + vol->target.path, (unsigned int) uid, + (unsigned int) gid); return -1; } if (chmod(vol->target.path, vol->target.perms.mode) < 0) { -- 1.7.10.4

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. 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(a)univention.de&gt; --- 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; 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. + * + * Unfortunately libvirt uses the value -1 to mean "current process", which + * on Linux32 gets converted to ALLONE:=+(2^32-1). + * + * 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 + */ if (virXPathNode("./owner", ctxt) == NULL) { perms->uid = (uid_t) -1; } else { - if (virXPathLong("number(./owner)", ctxt, &v) < 0) { + ret = virXPathNumber("number(./owner)", ctxt, &d); + if (ret == 0) { + if (d == (double) -1) { + perms->uid = (uid_t) -1; + } else { + perms->uid = (uid_t) (unsigned long) d; + if (perms->uid != d) { + ret = -2; + } + } + } + if (ret < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed owner element")); goto error; } - perms->uid = (int)v; } if (virXPathNode("./group", ctxt) == NULL) { perms->gid = (gid_t) -1; } else { - if (virXPathLong("number(./group)", ctxt, &v) < 0) { + ret = virXPathNumber("number(./group)", ctxt, &d); + if (ret == 0) { + if (d == (double) -1) { + perms->gid = (gid_t)-1; + } else { + perms->gid = (gid_t) (unsigned long) d; + if (perms->gid != d) { + ret = -2; + } + } + } + if (ret < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed group element")); goto error; } - perms->gid = (int)v; } /* NB, we're ignoring missing labels here - they'll simply inherit */ @@ -1060,10 +1091,18 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def) { virBufferAddLit(&buf," <permissions>\n"); virBufferAsprintf(&buf," <mode>0%o</mode>\n", def->target.perms.mode); - virBufferAsprintf(&buf," <owner>%d</owner>\n", - (int) def->target.perms.uid); - virBufferAsprintf(&buf," <group>%d</group>\n", - (int) def->target.perms.gid); + if (def->target.perms.uid == (uid_t) -1) { + virBufferAddLit(&buf, " <owner>-1</owner>\n"); + } else { + virBufferAsprintf(&buf, " <owner>%u</owner>\n", + (unsigned int) def->target.perms.uid); + } + if (def->target.perms.gid == (gid_t) -1) { + virBufferAddLit(&buf, " <group>-1</group>\n"); + } else { + virBufferAsprintf(&buf, " <group>%u</group>\n", + (unsigned int) def->target.perms.gid); + } if (def->target.perms.label) virBufferAsprintf(&buf," <label>%s</label>\n", @@ -1313,11 +1352,18 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," <permissions>\n"); virBufferAsprintf(buf," <mode>0%o</mode>\n", def->perms.mode); - virBufferAsprintf(buf," <owner>%u</owner>\n", - (unsigned int) def->perms.uid); - virBufferAsprintf(buf," <group>%u</group>\n", - (unsigned int) def->perms.gid); - + if (def->perms.uid == (uid_t) -1) { + virBufferAddLit(buf, " <owner>-1</owner>\n"); + } else { + virBufferAsprintf(buf, " <owner>%u</owner>\n", + (unsigned int) def->perms.uid); + } + if (def->perms.gid == (gid_t) -1) { + virBufferAddLit(buf, " <group>-1</group>\n"); + } else { + virBufferAsprintf(buf, " <group>%u</group>\n", + (unsigned int) def->perms.gid); + } if (def->perms.label) virBufferAsprintf(buf," <label>%s</label>\n", -- 1.7.10.4

On 02/22/2013 09:55 AM, Philipp Hahn wrote: I like the standardized name INT_MAX better than the invented name ALLONE. 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). 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