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@xxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list