Use mode_t in open and friends

Message ID 87k3afhsua.fsf@rasmusvillemoes.dk
State Superseded
Headers

Commit Message

Rasmus Villemoes April 24, 2014, 11:25 a.m. UTC
  The sole varargs argument to open and friends has type mode_t, not int.

Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
---
 io/open.c                                | 4 ++--
 io/open64.c                              | 4 ++--
 io/openat.c                              | 4 ++--
 io/openat64.c                            | 4 ++--
 sysdeps/posix/open64.c                   | 4 ++--
 sysdeps/unix/sysv/linux/generic/open.c   | 8 ++++----
 sysdeps/unix/sysv/linux/generic/open64.c | 4 ++--
 sysdeps/unix/sysv/linux/open64.c         | 4 ++--
 8 files changed, 18 insertions(+), 18 deletions(-)
  

Comments

Andreas Schwab April 24, 2014, 12:30 p.m. UTC | #1
Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

> The sole varargs argument to open and friends has type mode_t, not int.

Varargs use the promoted type, but mode_t traditionally has been
unsigned short.

Andreas.
  
Rasmus Villemoes April 24, 2014, 1:06 p.m. UTC | #2
Andreas Schwab <schwab@linux-m68k.org> writes:

> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
>
>> The sole varargs argument to open and friends has type mode_t, not int.
>
> Varargs use the promoted type, but mode_t traditionally has been
> unsigned short.

Makes sense. But then sysdeps/unix/sysv/linux/mq_open.c and
sysdeps/unix/sysv/linux/openat.c are inconsistent with the other files
in sysdeps/unix/sysv/linux/. 

(Also, mode_t is actually unsigned int on linux/x86_64.)

Rasmus
  
Paul Eggert April 24, 2014, 2:06 p.m. UTC | #3
gnulib attacks this problem by defining a macro PROMOTED_MODE_T that 
expands to 'int' on platforms where mode_t is narrower than int, and to 
'mode_t' otherwise.  This supports 'mode = va_arg (ap, 
PROMOTED_MODE_T);'.  glibc could do something similar.
  
Rich Felker April 25, 2014, 6:42 p.m. UTC | #4
On Thu, Apr 24, 2014 at 07:06:58AM -0700, Paul Eggert wrote:
> gnulib attacks this problem by defining a macro PROMOTED_MODE_T that
> expands to 'int' on platforms where mode_t is narrower than int, and
> to 'mode_t' otherwise.  This supports 'mode = va_arg (ap,
> PROMOTED_MODE_T);'.  glibc could do something similar.

I thought glibc always had mode_t with rank >= int, but if not,
something like __typeof__(+(mode_t)0) would work just fine, since
glibc depends on GNU C extensions like __typeof__ anyway.

Rich
  
Ondrej Bilka May 3, 2014, 9:28 a.m. UTC | #5
On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote:
> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
> 
> > The sole varargs argument to open and friends has type mode_t, not int.
> 
> Varargs use the promoted type, but mode_t traditionally has been
> unsigned short.
> 
So which solution do you prefer? Adding a custom type for that as
suggested in sibling threads? What about just adding a cast like

mode = (mode_t) va_arg (arg, int);
  
Pedro Alves May 3, 2014, 4:52 p.m. UTC | #6
On 05/03/2014 10:28 AM, Ondřej Bílka wrote:
> On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote:
>> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
>>
>>> The sole varargs argument to open and friends has type mode_t, not int.
>>
>> Varargs use the promoted type, but mode_t traditionally has been
>> unsigned short.
>>
> So which solution do you prefer? Adding a custom type for that as
> suggested in sibling threads? What about just adding a cast like
> 
> mode = (mode_t) va_arg (arg, int);
> 

Might be overkill for this instance alone, but another option
would be letting the compiler do the work.  It avoids having
to care whether mode_t might be wider than int, or unsigned.

Can be done with C11's _Generic, or GCC's
__builtin_choose_expr/__builtin_types_compatible_p.

Here's how it might look with the latter:

mode = __unpromoted_va_arg (arg, mode_t);

#define __maybe_va_arg(list, type, unpromoted, promoted, else_expr)	  \
  __builtin_choose_expr (__builtin_types_compatible_p (type, unpromoted), \
			 va_arg (list, promoted),			  \
			 else_expr)

/* Like va_arg, but accepts unpromoted types.  */
#define __promoted_va_arg(list, type)					\
  (type) __maybe_va_arg (list, type, float, double,			\
	 __maybe_va_arg (list, type, char, int,				\
	 __maybe_va_arg (list, type, short int, int,			\
	 __maybe_va_arg (list, type, unsigned char, unsigned int,	\
	 __maybe_va_arg (list, type, unsigned short int, unsigned int,	\
	 va_arg (list, type))))))
  
Rasmus Villemoes May 6, 2014, 9:36 a.m. UTC | #7
Pedro Alves <palves@redhat.com> writes:

> On 05/03/2014 10:28 AM, Ondřej Bílka wrote:
>> On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote:
>>> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
>>>
>>>> The sole varargs argument to open and friends has type mode_t, not int.
>>>
>>> Varargs use the promoted type, but mode_t traditionally has been
>>> unsigned short.
>>>
>> So which solution do you prefer? Adding a custom type for that as
>> suggested in sibling threads? What about just adding a cast like
>> 
>> mode = (mode_t) va_arg (arg, int);
>>

That requires sizeof(mode_t) <= sizeof(int) - which (I think) is the
case on all current and probably also all future platforms, but nothing
guarantees it.

> Might be overkill for this instance alone, but another option
> would be letting the compiler do the work.

Yes, that is definitely the best way. It would be nice if gcc exposed
something like __builtin_promoted_type(T) so one could simply say
va_arg(arg, __builtin_promoted_type(mode_t)); or even better if gcc had
a switch to simply DTRT when it encounters a va_arg with a
non-self-promoting type. gcc certainly already contains the
infrastructure to do this.

> Can be done with C11's _Generic, or GCC's
> __builtin_choose_expr/__builtin_types_compatible_p.
>
> Here's how it might look with the latter:
>
> mode = __unpromoted_va_arg (arg, mode_t);
>
> #define __maybe_va_arg(list, type, unpromoted, promoted, else_expr)	  \
>   __builtin_choose_expr (__builtin_types_compatible_p (type, unpromoted), \
> 			 va_arg (list, promoted),			  \
> 			 else_expr)
>
> /* Like va_arg, but accepts unpromoted types.  */
> #define __promoted_va_arg(list, type)					\
>   (type) __maybe_va_arg (list, type, float, double,			\
> 	 __maybe_va_arg (list, type, char, int,				\
> 	 __maybe_va_arg (list, type, short int, int,			\
> 	 __maybe_va_arg (list, type, unsigned char, unsigned int,	\
> 	 __maybe_va_arg (list, type, unsigned short int, unsigned int,	\
> 	 va_arg (list, type))))))

This seems to solve it, but as you say, it may be overkill. I don't
think there are functions other than open and derivatives with this
legacy problem. Since mode_t is guaranteed to be an integer type, I
think

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

would be sufficient. 

Rasmus
  
Ondrej Bilka May 16, 2014, 8:40 p.m. UTC | #8
On Tue, May 06, 2014 at 11:36:53AM +0200, Rasmus Villemoes wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
> > On 05/03/2014 10:28 AM, Ondřej Bílka wrote:
> >> On Thu, Apr 24, 2014 at 02:30:21PM +0200, Andreas Schwab wrote:
> >>> Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:
> >>>
> >>>> The sole varargs argument to open and friends has type mode_t, not int.
> >>>
> >>> Varargs use the promoted type, but mode_t traditionally has been
> >>> unsigned short.
> >>>
> >> So which solution do you prefer? Adding a custom type for that as
> >> suggested in sibling threads? What about just adding a cast like
> >> 
> >> mode = (mode_t) va_arg (arg, int);
> >>
> 
> That requires sizeof(mode_t) <= sizeof(int) - which (I think) is the
> case on all current and probably also all future platforms, but nothing
> guarantees it.
> 
> > Might be overkill for this instance alone, but another option
> > would be letting the compiler do the work.
> 
> Yes, that is definitely the best way. It would be nice if gcc exposed
> something like __builtin_promoted_type(T) so one could simply say
> va_arg(arg, __builtin_promoted_type(mode_t)); or even better if gcc had
> a switch to simply DTRT when it encounters a va_arg with a
> non-self-promoting type. gcc certainly already contains the
> infrastructure to do this.
> 
Could you prepare it as v2 of this patch?
  
Mike Frysinger Aug. 3, 2014, 3:47 p.m. UTC | #9
logistically speaking, have you signed FSF copyright papers for glibc ?
-mike
  
Rasmus Villemoes Aug. 3, 2014, 3:57 p.m. UTC | #10
Mike Frysinger <vapier@gentoo.org> writes:

> logistically speaking, have you signed FSF copyright papers for glibc
> ?

No. Where and how do I do that? Will scan+email be sufficient, or does
the process involve snailmail?

Rasmus
  
Mike Frysinger Aug. 4, 2014, 12:32 a.m. UTC | #11
On Sun 03 Aug 2014 17:57:27 Rasmus Villemoes wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> > logistically speaking, have you signed FSF copyright papers for glibc
> > ?
> 
> No. Where and how do I do that? Will scan+email be sufficient, or does
> the process involve snailmail?

they're always changing the process ;).  the bootstrap is def over e-mail, and 
the assignment people will provide direction after that.

start here:
https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment
you'll want the request-assign.future.  just fill that out and e-mail it to 
assign@gnu.org.

please make sure to read over the papers they send you so you're comfortable 
with what it entails.  the scope should be limited to changes to the glibc 
project in case that helps.
-mike
  
Rasmus Villemoes Sept. 2, 2014, 4:28 p.m. UTC | #12
On Mon, Aug 04 2014, Mike Frysinger <vapier@gentoo.org> wrote:

> On Sun 03 Aug 2014 17:57:27 Rasmus Villemoes wrote:
>> Mike Frysinger <vapier@gentoo.org> writes:
>> > logistically speaking, have you signed FSF copyright papers for glibc
>> > ?
>> 
>> No. Where and how do I do that? Will scan+email be sufficient, or does
>> the process involve snailmail?
>
> they're always changing the process ;).  the bootstrap is def over e-mail, and 
> the assignment people will provide direction after that.
>
> start here:
> https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment
> you'll want the request-assign.future.  just fill that out and e-mail it to 
> assign@gnu.org.

Bootstrapping went fine (almost immediate response). I then sent the 
paperwork to FSF, by now almost a month ago, but then haven't heard
anything. Nor have they replied to my email where I asked if the snail
mail had arrived. What would be the next logical step?

Rasmus
  
Rasmus Villemoes Sept. 5, 2014, 8:21 a.m. UTC | #13
On Tue, Sep 02 2014, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote:

> Bootstrapping went fine (almost immediate response). I then sent the 
> paperwork to FSF, by now almost a month ago, but then haven't heard
> anything. Nor have they replied to my email where I asked if the snail
> mail had arrived. What would be the next logical step?

That helped. The paperwork should now be in order, so can I get someone
to look at

http://patchwork.sourceware.org/patch/2292/ (latest version mode_t patch)
http://patchwork.sourceware.org/patch/1306/ (sendmmsg prototype fix)
http://patchwork.sourceware.org/patch/1048/ (sys/time.h + test case)
http://thread.gmane.org/gmane.comp.lib.glibc.alpha/41725 (eventfd
prototype fix)

Thanks,
Rasmus
  

Patch

diff --git a/io/open.c b/io/open.c
index 24aa380..5be57a1 100644
--- a/io/open.c
+++ b/io/open.c
@@ -30,7 +30,7 @@  __libc_open (file, oflag)
      const char *file;
      int oflag;
 {
-  int mode;
+  mode_t mode;
 
   if (file == NULL)
     {
@@ -42,7 +42,7 @@  __libc_open (file, oflag)
     {
       va_list arg;
       va_start(arg, oflag);
-      mode = va_arg(arg, int);
+      mode = va_arg(arg, mode_t);
       va_end(arg);
     }
 
diff --git a/io/open64.c b/io/open64.c
index 3f3d2e8..b740ab6 100644
--- a/io/open64.c
+++ b/io/open64.c
@@ -28,7 +28,7 @@  __libc_open64 (file, oflag)
      const char *file;
      int oflag;
 {
-  int mode;
+  mode_t mode;
 
   if (file == NULL)
     {
@@ -40,7 +40,7 @@  __libc_open64 (file, oflag)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }
 
diff --git a/io/openat.c b/io/openat.c
index 2d82270..aac603c 100644
--- a/io/openat.c
+++ b/io/openat.c
@@ -38,7 +38,7 @@  __openat (fd, file, oflag)
      const char *file;
      int oflag;
 {
-  int mode;
+  mode_t mode;
 
   if (file == NULL)
     {
@@ -64,7 +64,7 @@  __openat (fd, file, oflag)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }
 
diff --git a/io/openat64.c b/io/openat64.c
index c0c4e19..1f126fc 100644
--- a/io/openat64.c
+++ b/io/openat64.c
@@ -31,7 +31,7 @@  __openat64 (fd, file, oflag)
      const char *file;
      int oflag;
 {
-  int mode;
+  mode_t mode;
 
   if (file == NULL)
     {
@@ -57,7 +57,7 @@  __openat64 (fd, file, oflag)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }
 
diff --git a/sysdeps/posix/open64.c b/sysdeps/posix/open64.c
index 64d192a..9364454 100644
--- a/sysdeps/posix/open64.c
+++ b/sysdeps/posix/open64.c
@@ -24,13 +24,13 @@ 
 int
 __libc_open64 (const char *file, int oflag, ...)
 {
-  int mode = 0;
+  mode_t mode = 0;
 
   if (oflag & O_CREAT)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }
 
diff --git a/sysdeps/unix/sysv/linux/generic/open.c b/sysdeps/unix/sysv/linux/generic/open.c
index 4f73fa0..d243933 100644
--- a/sysdeps/unix/sysv/linux/generic/open.c
+++ b/sysdeps/unix/sysv/linux/generic/open.c
@@ -27,13 +27,13 @@ 
 int
 __libc_open (const char *file, int oflag, ...)
 {
-  int mode = 0;
+  mode_t mode = 0;
 
   if (oflag & O_CREAT)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }
 
@@ -57,13 +57,13 @@  weak_alias (__libc_open, open)
 int
 __open_nocancel (const char *file, int oflag, ...)
 {
-  int mode = 0;
+  mode_t mode = 0;
 
   if (oflag & O_CREAT)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }
 
diff --git a/sysdeps/unix/sysv/linux/generic/open64.c b/sysdeps/unix/sysv/linux/generic/open64.c
index 93d79e3..e367e12 100644
--- a/sysdeps/unix/sysv/linux/generic/open64.c
+++ b/sysdeps/unix/sysv/linux/generic/open64.c
@@ -27,13 +27,13 @@ 
 int
 __libc_open64 (const char *file, int oflag, ...)
 {
-  int mode = 0;
+  mode_t mode = 0;
 
   if (oflag & O_CREAT)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }
 
diff --git a/sysdeps/unix/sysv/linux/open64.c b/sysdeps/unix/sysv/linux/open64.c
index 0d63806..442a2ef 100644
--- a/sysdeps/unix/sysv/linux/open64.c
+++ b/sysdeps/unix/sysv/linux/open64.c
@@ -26,13 +26,13 @@ 
 int
 __libc_open64 (const char *file, int oflag, ...)
 {
-  int mode = 0;
+  mode_t mode = 0;
 
   if (oflag & O_CREAT)
     {
       va_list arg;
       va_start (arg, oflag);
-      mode = va_arg (arg, int);
+      mode = va_arg (arg, mode_t);
       va_end (arg);
     }