ZFS compiler warnings

[Matthew Ahrens]

On Fri, Apr 10, 2015 at 4:58 PM, Garrett Wollman <wollman at csail.mit.edu>
wrote:

> I notice on my 10.1 kernel builds warnings like the following:
>
> /usr/src/sys/modules/zfs/../../cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c:2855:12:
> warning: comparison of constant -1 with expression of type 'zfs_prop_t' is
> always true [-Wtautological-constant-out-of-range-compare]
>                 if (prop != ZPROP_INVAL && !zfs_prop_inheritable(prop))
>                     ~~~~ ^  ~~~~~~~~~~~
> /usr/src/sys/modules/zfs/../../cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c:3810:11:
> warning: comparison of constant -1 with expression of type 'zfs_prop_t' is
> always false [-Wtautological-constant-out-of-range-compare]
>         if (prop == ZPROP_INVAL) {
>             ~~~~ ^  ~~~~~~~~~~~
>
> The warnings indicate that these checks are being optimized away by
> clang.  The code in head is the same.  Does anyone have an idea of how
> this should be fixed, in a way that would be acceptable upstream?  (Do
> we have someone who is the "official" liaison with the OpenZFS
> project?)
>

You change makes sense to me.  I'd be happy to see it go upstream in
illumos too.

--matt


>
> My guess is that the definition of the zfs_prop_t enum itself needs to
> include these two constants explicitly -- but that also has the effect
> of changing the type of ZPROP_INVAL, which then means that
> zpool_prop_t needs to have its own version of "invalid property"
> defined, and that then requires cascading changes in a bunch of other
> places.
>
> The following is a minimal change that fixes the warnings in the
> kernel alone (beware whitespace lossage) -- I haven't checked the
> userland side:
>
> Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c
> ===================================================================
> --- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c        (revision
> 281265)
> +++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c        (working
> copy)
> @@ -332,7 +332,7 @@
>                 zprop_source_t src = ZPROP_SRC_DEFAULT;
>                 zpool_prop_t prop;
>
> -               if ((prop = zpool_name_to_prop(za.za_name)) == ZPROP_INVAL)
> +               if ((prop = zpool_name_to_prop(za.za_name)) ==
> ZPOOL_PROP_INVAL)
>                         continue;
>
>                 switch (za.za_integer_length) {
> @@ -422,7 +422,7 @@
>                 zpool_prop_t prop = zpool_name_to_prop(propname);
>
>                 switch (prop) {
> -               case ZPROP_INVAL:
> +               case ZPOOL_PROP_INVAL:
>                         if (!zpool_prop_feature(propname)) {
>                                 error = SET_ERROR(EINVAL);
>                                 break;
> @@ -662,7 +662,7 @@
>                     prop == ZPOOL_PROP_READONLY)
>                         continue;
>
> -               if (prop == ZPOOL_PROP_VERSION || prop == ZPROP_INVAL) {
> +               if (prop == ZPOOL_PROP_VERSION || prop ==
> ZPOOL_PROP_INVAL) {
>                         uint64_t ver;
>
>                         if (prop == ZPOOL_PROP_VERSION) {
> @@ -6308,7 +6308,7 @@
>                 spa_feature_t fid;
>
>                 switch (prop = zpool_name_to_prop(nvpair_name(elem))) {
> -               case ZPROP_INVAL:
> +               case ZPOOL_PROP_INVAL:
>                         /*
>                          * We checked this earlier in spa_prop_validate().
>                          */
> Index: sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h
> ===================================================================
> --- sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h        (revision
> 281265)
> +++ sys/cddl/contrib/opensolaris/uts/common/sys/fs/zfs.h        (working
> copy)
> @@ -152,7 +152,9 @@
>         ZFS_PROP_SNAPSHOT_COUNT,
>         ZFS_PROP_REDUNDANT_METADATA,
>         ZFS_PROP_PREV_SNAP,
> -       ZFS_NUM_PROPS
> +       ZFS_NUM_PROPS,
> +       ZPROP_INVAL = -1,
> +       ZPROP_CONT = -2
>  } zfs_prop_t;
>
>  typedef enum {
> @@ -196,14 +198,18 @@
>         ZPOOL_PROP_FREEING,
>         ZPOOL_PROP_FRAGMENTATION,
>         ZPOOL_PROP_LEAKED,
> -       ZPOOL_NUM_PROPS
> +       ZPOOL_NUM_PROPS,
> +       ZPOOL_PROP_INVAL = -1,
> +       ZPOOL_PROP_CONT = -2
>  } zpool_prop_t;
>
>  /* Small enough to not hog a whole line of printout in zpool(1M). */
>  #define        ZPROP_MAX_COMMENT       32
>
> +/*
>  #define        ZPROP_CONT              -2
>  #define        ZPROP_INVAL             -1
> +*/
>
>  #define        ZPROP_VALUE             "value"
>  #define        ZPROP_SOURCE            "source"
>
> There is also one spot in zio_checksum.c that has a similar problem,
> but in this case the issue is that it assumes that an out-of-range
> value can be bitwise-or'ed with a small enum.  (The C standard allows
> the compiler to use an eight-bit integer type as the underlying type
> of 'enum zio_checksum', so to bitwise-or it with ZIO_CHECKSUM_VERIFY,
> defined to be 256, is undefined.)  A similar change eliminates this
> warning.  (The effect in both cases is to convince the compiler to
> model the enum with a larger underlying type.)
>
> -GAWollman
> _______________________________________________
> freebsd-fs at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"
>