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

Re: [libvirt] [PATCH 3/4] storage: cast -1 for uid_t|gid_t



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

Attachment: signature.asc
Description: OpenPGP digital signature


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