On 02/22/2013 09:41 AM, Philipp Hahn wrote:
uid_t and gid_t are opaque types, ranging from s32 to u32 to u64.
Or even s16 and u16 in older Unix.
Explicitly cast the magic -1 to the appropriate type.
Necessary in some cases, redundant in others; but overall good to be consistent.
Signed-off-by: Philipp Hahn <hahn@univention.de> --- 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;
Redundant. C guarantees that -1 will promote correctly via assignment to any size integer, signed or unsigned.
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;
Redundant.
} 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;
Redundant.
} 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;
Necessary - ternary operator rules for integer promotion are not necessarily intuitive; and it is conceivable that mixing u16 with int can produce wrong results. Forcing both sides of the expression to be uid_t is indeed worthwhile.
+ gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1;
Ditto on being necessary.
+ if (((uid != (uid_t) -1) || (gid != (gid_t) -1))
And comparison also needs the cast - definitely necessary.
&& (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)
Necessary.
&& (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))
Necessary.
&& (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)) {
Redundant (integer promotion via prototyped function calls does the right thing). Since the patch is already pushed, I'm fine leaving the redundant parts in. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org