Message ID | 20200211165256.21848-1-lukma@denx.de |
---|---|
State | New |
Headers | show |
Series | [1/2] y2038: linux: Provide __mq_timedsend_time64 implementation | expand |
On Feb 11 2020, Lukasz Majewski wrote: > diff --git a/include/mqueue.h b/include/mqueue.h > index 3c66f1711e..3de890905c 100644 > --- a/include/mqueue.h > +++ b/include/mqueue.h > @@ -10,4 +10,13 @@ extern __typeof (mq_timedreceive) __mq_timedreceive __nonnull ((2, 5)); > hidden_proto (__mq_timedreceive) > hidden_proto (mq_setattr) > # endif > +#if __TIMESIZE == 64 > +# define __mq_timedsend_time64 __mq_timedsend > +#else > +#include <include/time.h> I think struct __timespec64 should be moved to a separate file, so that we do not need to pull in the whole <time.h> namespace here. Andreas.
On Tue, 11 Feb 2020, Lukasz Majewski wrote: > +int > +__mq_timedsend (mqd_t mqdes, const char *msg_ptr, size_t msg_len, > + unsigned int msg_prio, const struct timespec *abs_timeout) > +{ > + struct __timespec64 ts64; > + if (abs_timeout) > + ts64 = valid_timespec_to_timespec64 (*abs_timeout); > + > + return __mq_timedsend_time64 (mqdes, msg_ptr, msg_len, msg_prio, > + abs_timeout ? &ts64 : NULL); > } The indentation seems wrong here. Likewise in patch 2.
Hi Joseph, > On Tue, 11 Feb 2020, Lukasz Majewski wrote: > > > +int > > +__mq_timedsend (mqd_t mqdes, const char *msg_ptr, size_t msg_len, > > + unsigned int msg_prio, const struct timespec > > *abs_timeout) +{ > > + struct __timespec64 ts64; > > + if (abs_timeout) > > + ts64 = valid_timespec_to_timespec64 (*abs_timeout); > > + > > + return __mq_timedsend_time64 (mqdes, msg_ptr, msg_len, > > msg_prio, > > + abs_timeout ? &ts64 : NULL); > > } > > The indentation seems wrong here. Likewise in patch 2. > Thanks for spotting it. I will fix it in v2. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Andreas, > On Feb 11 2020, Lukasz Majewski wrote: > > > diff --git a/include/mqueue.h b/include/mqueue.h > > index 3c66f1711e..3de890905c 100644 > > --- a/include/mqueue.h > > +++ b/include/mqueue.h > > @@ -10,4 +10,13 @@ extern __typeof (mq_timedreceive) > > __mq_timedreceive __nonnull ((2, 5)); hidden_proto > > (__mq_timedreceive) hidden_proto (mq_setattr) > > # endif > > +#if __TIMESIZE == 64 > > +# define __mq_timedsend_time64 __mq_timedsend > > +#else > > +#include <include/time.h> > > I think struct __timespec64 should be moved to a separate file, so > that we do not need to pull in the whole <time.h> namespace here. > Just to clarify - the time.h here is from ./include glibc sources directory, so it is "internal" for glibc. In other words - I would like to know if your comment was regarding polluting the glibc internal namespace? > Andreas. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Feb 12 2020, Lukasz Majewski wrote: > Just to clarify - the time.h here is from ./include glibc sources > directory, so it is "internal" for glibc. But it is a wrapper around <time.h>. > In other words - I would like to know if your comment was regarding > polluting the glibc internal namespace? It's not about polluting, but since the #include is conditional, we have inconsistent environment between configurations, which isn't nice. By moving struct __timespec64 to <include/bits/struct/timespec.h> we can avoid that inconsistency. Andreas.
Hi Andreas, > On Feb 12 2020, Lukasz Majewski wrote: > > > Just to clarify - the time.h here is from ./include glibc sources > > directory, so it is "internal" for glibc. > > But it is a wrapper around <time.h>. Yes, indeed - it includes <time/time.h> > > > In other words - I would like to know if your comment was regarding > > polluting the glibc internal namespace? > > It's not about polluting, but since the #include is conditional, we > have inconsistent environment between configurations, which isn't > nice. Ach... I see. > > By moving struct __timespec64 to <include/bits/struct/timespec.h> we > can avoid that inconsistency. Maybe <include/bits/types/struct___timespec64.h> would be a better place/name? In that way we would follow other "internal" structs definitions - i.e. <include/bits/types/struct_timespec.h> (which only has #include·<time/bits/types/struct_timespec.h>) > > Andreas. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
* Lukasz Majewski: >> By moving struct __timespec64 to <include/bits/struct/timespec.h> we >> can avoid that inconsistency. > > Maybe <include/bits/types/struct___timespec64.h> would be a better > place/name? bits/ headers should be installed headers. If it is just an internal type, it should not go into a bits/ header, but a header under include/ or maybe sysdeps/generic/. The latter is useful if sysdeps overrides are expected.
On Feb 12 2020, Lukasz Majewski wrote: >> By moving struct __timespec64 to <include/bits/struct/timespec.h> we >> can avoid that inconsistency. > > Maybe <include/bits/types/struct___timespec64.h> would be a better > place/name? By adding it to <include/bits/struct/timespec.h> it would be included automatically. Andreas.
Hi Florian, > * Lukasz Majewski: > > >> By moving struct __timespec64 to <include/bits/struct/timespec.h> > >> we can avoid that inconsistency. > > > > Maybe <include/bits/types/struct___timespec64.h> would be a better > > place/name? > > bits/ headers should be installed headers. If it is just an internal > type, it should not go into a bits/ header, but a header under > include/ or maybe sysdeps/generic/. The latter is useful if sysdeps > overrides are expected. the idea would be to place it under ./include/bits/ in the glibc source directory, so it would be an "internal" header. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Wed, 12 Feb 2020 11:44:20 +0100 Andreas Schwab <schwab@suse.de> wrote: > On Feb 12 2020, Lukasz Majewski wrote: > > >> By moving struct __timespec64 to <include/bits/struct/timespec.h> > >> we can avoid that inconsistency. > > > > Maybe <include/bits/types/struct___timespec64.h> would be a better > > place/name? > > By adding it to <include/bits/struct/timespec.h> it would be included > automatically. > Then it would be a great optimization (as __timespec64 is included in several places) (And I do guess that this - auto include - is related to the directory search patch in Makefiles?) > Andreas. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
* Lukasz Majewski: > Hi Florian, > >> * Lukasz Majewski: >> >> >> By moving struct __timespec64 to <include/bits/struct/timespec.h> >> >> we can avoid that inconsistency. >> > >> > Maybe <include/bits/types/struct___timespec64.h> would be a better >> > place/name? >> >> bits/ headers should be installed headers. If it is just an internal >> type, it should not go into a bits/ header, but a header under >> include/ or maybe sysdeps/generic/. The latter is useful if sysdeps >> overrides are expected. > > the idea would be to place it under ./include/bits/ in the glibc source > directory, so it would be an "internal" header. Okay, that part makes sense.
On Feb 12 2020, Lukasz Majewski wrote: > (And I do guess that this - auto include - is related to the directory > search patch in Makefiles?) For the same reason <time.h> finds include/time.h first during build. Andreas.
Hi Andreas, > On Feb 12 2020, Lukasz Majewski wrote: > > > (And I do guess that this - auto include - is related to the > > directory search patch in Makefiles?) > > For the same reason <time.h> finds include/time.h first during build. > > Andreas. > I've added the struct __timespec64 definition to ./include/bits/struct/timespec.h and I had build errors on ./include/time.h which uses struct __timespec64 massively. It requires explicit adding of #include <include/bits/struct/timespec.h> or <bits/struct/timespec.h> The same #include was required in ./include/mqueue.h to have proper struct __timespec64 declaration. It seems like the glibc build system fails to #include it automatically to internal headers (like ./include/time.h and ./include/mqueue.h). IMHO, it would be better to add definition of struct __timespec64 into ./include/bits/types/struct___timespec64.h (which is intended for internal glibc use). Such approach would follow what we do have now in ./include/time.h with # include <bits/types/struct_timeval.h> Or what I'm missing here? Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Feb 12 2020, Lukasz Majewski wrote: > I've added the struct __timespec64 definition to > ./include/bits/struct/timespec.h > > > and I had build errors on ./include/time.h which uses struct > __timespec64 massively. > It requires explicit adding of #include > <include/bits/struct/timespec.h> or <bits/struct/timespec.h> This works for me: diff --git a/include/bits/types/struct_timespec.h b/include/bits/types/struct_timespec.h index c27417cfd5..de1cf44311 100644 --- a/include/bits/types/struct_timespec.h +++ b/include/bits/types/struct_timespec.h @@ -1 +1,30 @@ +#ifndef _STRUCT_TIMESPEC #include <time/bits/types/struct_timespec.h> + +#ifndef _ISOMAC +# include <endian.h> +# if __TIMESIZE == 64 +# define __timespec64 timespec +# else +/* The glibc Y2038-proof struct __timespec64 structure for a time value. + To keep things Posix-ish, we keep the nanoseconds field a 32-bit + signed long, but since the Linux field is a 64-bit signed int, we + pad our tv_nsec with a 32-bit unnamed bit-field padding. + + As a general rule the Linux kernel is ignoring upper 32 bits of + tv_nsec field. */ +struct __timespec64 +{ + __time64_t tv_sec; /* Seconds */ +# if BYTE_ORDER == BIG_ENDIAN + __int32_t :32; /* Padding */ + __int32_t tv_nsec; /* Nanoseconds */ +# else + __int32_t tv_nsec; /* Nanoseconds */ + __int32_t :32; /* Padding */ +# endif +}; +# endif +#endif + +#endif diff --git a/include/time.h b/include/time.h index 73f66277ac..f806bd8f0f 100644 --- a/include/time.h +++ b/include/time.h @@ -6,7 +6,6 @@ # include <bits/types/locale_t.h> # include <stdbool.h> # include <time/mktime-internal.h> -# include <endian.h> # include <time-clockid.h> # include <sys/time.h> @@ -60,29 +59,6 @@ extern void __tzset_parse_tz (const char *tz) attribute_hidden; extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime) __THROW attribute_hidden; -#if __TIMESIZE == 64 -# define __timespec64 timespec -#else -/* The glibc Y2038-proof struct __timespec64 structure for a time value. - To keep things Posix-ish, we keep the nanoseconds field a 32-bit - signed long, but since the Linux field is a 64-bit signed int, we - pad our tv_nsec with a 32-bit unnamed bit-field padding. - - As a general rule the Linux kernel is ignoring upper 32 bits of - tv_nsec field. */ -struct __timespec64 -{ - __time64_t tv_sec; /* Seconds */ -# if BYTE_ORDER == BIG_ENDIAN - __int32_t :32; /* Padding */ - __int32_t tv_nsec; /* Nanoseconds */ -# else - __int32_t tv_nsec; /* Nanoseconds */ - __int32_t :32; /* Padding */ -# endif -}; -#endif - #if __TIMESIZE == 64 # define __itimerspec64 itimerspec #else Andreas.
Hi Andreas, > On Feb 12 2020, Lukasz Majewski wrote: > > > I've added the struct __timespec64 definition to > > ./include/bits/struct/timespec.h > > > > > > and I had build errors on ./include/time.h which uses struct > > __timespec64 massively. > > It requires explicit adding of #include > > <include/bits/struct/timespec.h> or <bits/struct/timespec.h> > Apparently I had wrong paths. > This works for me: > > diff --git a/include/bits/types/struct_timespec.h > b/include/bits/types/struct_timespec.h index c27417cfd5..de1cf44311 > 100644 --- a/include/bits/types/struct_timespec.h > +++ b/include/bits/types/struct_timespec.h The question is about naming convention - include/bits/types/struct_timespec.h -> suggest that we do have here the "internal" re-definition of struct timespec but IMHO we do introduce another "internal type" - namely struct __timespec64 Hence the below code shall be put to: include/bits/types/struct__timespec64.h to have the distinction between internally used struct timespec and struct __timespec64 (which would be used explicitly on archs with __WORDSIZE==32 (e.g. arm32, rv32) Am I right here? > @@ -1 +1,30 @@ > +#ifndef _STRUCT_TIMESPEC > #include <time/bits/types/struct_timespec.h> > + > +#ifndef _ISOMAC > +# include <endian.h> > +# if __TIMESIZE == 64 > +# define __timespec64 timespec > +# else > +/* The glibc Y2038-proof struct __timespec64 structure for a time > value. > + To keep things Posix-ish, we keep the nanoseconds field a 32-bit > + signed long, but since the Linux field is a 64-bit signed int, we > + pad our tv_nsec with a 32-bit unnamed bit-field padding. > + > + As a general rule the Linux kernel is ignoring upper 32 bits of > + tv_nsec field. */ > +struct __timespec64 > +{ > + __time64_t tv_sec; /* Seconds */ > +# if BYTE_ORDER == BIG_ENDIAN > + __int32_t :32; /* Padding */ > + __int32_t tv_nsec; /* Nanoseconds */ > +# else > + __int32_t tv_nsec; /* Nanoseconds */ > + __int32_t :32; /* Padding */ > +# endif > +}; > +# endif > +#endif > + > +#endif > diff --git a/include/time.h b/include/time.h > index 73f66277ac..f806bd8f0f 100644 > --- a/include/time.h > +++ b/include/time.h > @@ -6,7 +6,6 @@ > # include <bits/types/locale_t.h> > # include <stdbool.h> > # include <time/mktime-internal.h> > -# include <endian.h> > # include <time-clockid.h> > # include <sys/time.h> > > @@ -60,29 +59,6 @@ extern void __tzset_parse_tz (const char *tz) > attribute_hidden; extern void __tz_compute (__time64_t timer, struct > tm *tm, int use_localtime) __THROW attribute_hidden; > > -#if __TIMESIZE == 64 > -# define __timespec64 timespec > -#else > -/* The glibc Y2038-proof struct __timespec64 structure for a time > value. > - To keep things Posix-ish, we keep the nanoseconds field a 32-bit > - signed long, but since the Linux field is a 64-bit signed int, we > - pad our tv_nsec with a 32-bit unnamed bit-field padding. > - > - As a general rule the Linux kernel is ignoring upper 32 bits of > - tv_nsec field. */ > -struct __timespec64 > -{ > - __time64_t tv_sec; /* Seconds */ > -# if BYTE_ORDER == BIG_ENDIAN > - __int32_t :32; /* Padding */ > - __int32_t tv_nsec; /* Nanoseconds */ > -# else > - __int32_t tv_nsec; /* Nanoseconds */ > - __int32_t :32; /* Padding */ > -# endif > -}; > -#endif > - > #if __TIMESIZE == 64 > # define __itimerspec64 itimerspec > #else > > Andreas. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Feb 12 2020, Lukasz Majewski wrote: > The question is about naming convention - > > include/bits/types/struct_timespec.h -> suggest that we do have here > the "internal" re-definition of struct timespec but IMHO we do > introduce another "internal type" - namely struct __timespec64 > > Hence the below code shall be put to: > include/bits/types/struct__timespec64.h That would be ok with me either way. Andreas.
Hi Andreas, > On Feb 12 2020, Lukasz Majewski wrote: > > > The question is about naming convention - > > > > include/bits/types/struct_timespec.h -> suggest that we do have here > > the "internal" re-definition of struct timespec but IMHO we do > > introduce another "internal type" - namely struct __timespec64 > > > > Hence the below code shall be put to: > > include/bits/types/struct__timespec64.h > > That would be ok with me either way. > > Andreas. > The same code put into include/bits/types/struct___timespec64.h file requires having the explicit #include in ./include/time.h (and mqueue.h). However, it works correctly (i.e. is included automatically) only when put into include/bits/types/struct_timespec.h I need to investigate it further... Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/include/mqueue.h b/include/mqueue.h index 3c66f1711e..3de890905c 100644 --- a/include/mqueue.h +++ b/include/mqueue.h @@ -10,4 +10,13 @@ extern __typeof (mq_timedreceive) __mq_timedreceive __nonnull ((2, 5)); hidden_proto (__mq_timedreceive) hidden_proto (mq_setattr) # endif +#if __TIMESIZE == 64 +# define __mq_timedsend_time64 __mq_timedsend +#else +#include <include/time.h> +extern int __mq_timedsend_time64 (mqd_t mqdes, const char *msg_ptr, + size_t msg_len, unsigned int msg_prio, + const struct __timespec64 *abs_timeout); +librt_hidden_proto (__mq_timedsend_time64) +#endif #endif diff --git a/sysdeps/unix/sysv/linux/mq_timedsend.c b/sysdeps/unix/sysv/linux/mq_timedsend.c index 888ec6744a..662309c365 100644 --- a/sysdeps/unix/sysv/linux/mq_timedsend.c +++ b/sysdeps/unix/sysv/linux/mq_timedsend.c @@ -22,12 +22,55 @@ /* Add message pointed by MSG_PTR to message queue MQDES, stop blocking on full message queue if ABS_TIMEOUT expires. */ int -__mq_timedsend (mqd_t mqdes, const char *msg_ptr, size_t msg_len, - unsigned int msg_prio, const struct timespec *abs_timeout) +__mq_timedsend_time64 (mqd_t mqdes, const char *msg_ptr, size_t msg_len, + unsigned int msg_prio, + const struct __timespec64 *abs_timeout) { +#ifdef __ASSUME_TIME64_SYSCALLS +# ifndef __NR_mq_timedsend_time64 +# define __NR_mq_timedsend_time64 __NR_mq_timedsend +# endif + return SYSCALL_CANCEL (mq_timedsend_time64, mqdes, msg_ptr, msg_len, msg_prio, + abs_timeout); +#else + int ret = SYSCALL_CANCEL (mq_timedsend_time64, mqdes, msg_ptr, msg_len, + msg_prio, abs_timeout); + if (ret == 0 || errno != ENOSYS) + return ret; + + struct timespec ts32; + if (abs_timeout) + { + if (! in_time_t_range (abs_timeout->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + ts32 = valid_timespec64_to_timespec (*abs_timeout); + } + return SYSCALL_CANCEL (mq_timedsend, mqdes, msg_ptr, msg_len, msg_prio, - abs_timeout); + abs_timeout ? &ts32 : NULL); +#endif +} + +#if __TIMESIZE != 64 +librt_hidden_def (__mq_timedsend_time64) + +int +__mq_timedsend (mqd_t mqdes, const char *msg_ptr, size_t msg_len, + unsigned int msg_prio, const struct timespec *abs_timeout) +{ + struct __timespec64 ts64; + if (abs_timeout) + ts64 = valid_timespec_to_timespec64 (*abs_timeout); + + return __mq_timedsend_time64 (mqdes, msg_ptr, msg_len, msg_prio, + abs_timeout ? &ts64 : NULL); } +#endif + hidden_def (__mq_timedsend) weak_alias (__mq_timedsend, mq_timedsend) hidden_weak (mq_timedsend)