Use mode_t in open and friends
Commit Message
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
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.
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
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.
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
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);
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))))))
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
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?
logistically speaking, have you signed FSF copyright papers for glibc ?
-mike
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
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
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
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
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -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);
}