Re: [Qemu-devel] [PATCH v2 12/20] 9p: darwin: Explicitly cast comparisons of mode_t with -1

[Eric Blake <eblake@redhat.com>]

On 05/31/2018 08:26 PM, Keno Fischer wrote:
> Comparisons of mode_t with -1 require an explicit cast, since mode_t
> is unsigned on Darwin.

It's not JUST that mode_t is unsigned (an unsigned int compares just 
fine against -1), but ALSO that mode_t has unspecified width.  That is, 
you cannot portably assume whether mode_t is smaller, equivalent, or 
larger rank than int.  If it is smaller, then you can't use mode_t in 
va_arg(), and mode_t will promote to signed int, whether or not mode_t 
is unsigned; but '((mode_t)-1) == -1' is going to be false if mode_t is 
unsigned (because the cast truncates the sign extension bits into a 
positive value).  Conversely, since mode_t can be larger than int 
(although I know of no such platform that does so), blindly using 'int' 
when trying to parse a mode_t argument through va_arg() will truncate bits.

So, for example, to portably write a wrapper around open(), you HAVE to 
use hairy constructions like:

mode = (sizeof (mode_t) < sizeof (int)
         ? va_arg (ap, int)
         : va_arg (ap, mode_t));

or, to avoid spurious compiler warnings on the branch not taken, define 
a macro learned at configure time.  This is what gnulib does, for example:

   AC_CACHE_CHECK([for promoted mode_t type], [gl_cv_promoted_mode_t], [
     dnl Assume mode_t promotes to 'int' if and only if it is smaller 
than 'int',
     dnl and to itself otherwise. This assumption is not guaranteed by 
the ISO C
     dnl standard, but we don't know of any real-world counterexamples.
     AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <sys/types.h>]],
       [[typedef int array[2 * (sizeof (mode_t) < sizeof (int)) - 1];]])],
       [gl_cv_promoted_mode_t='int'],
       [gl_cv_promoted_mode_t='mode_t'])
   ])
   AC_DEFINE_UNQUOTED([PROMOTED_MODE_T], [$gl_cv_promoted_mode_t],
     [Define to the type that is the result of default argument 
promotions of type mode_t.])

> +++ b/hw/9pfs/9p-local.c
> @@ -310,7 +310,7 @@ update_map_file:
>       if (credp->fc_gid != -1) {
>           gid = credp->fc_gid;
>       }
> -    if (credp->fc_mode != -1) {
> +    if (credp->fc_mode != (mode_t)-1) {

At any rate, this is the correct portability fix for this code.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org