Patchwork [v2,1/6] support: Add xclock_gettime

login
register
mail settings
Submitter Mike Crowe
Date April 7, 2019, 7:33 p.m.
Message ID <82e30caf3e52201a31b6f736fbad64ffd069ea3f.1554665560.git-series.mac@mcrowe.com>
Download mbox | patch
Permalink /patch/32184/
State New
Headers show

Comments

Mike Crowe - April 7, 2019, 7:33 p.m.
* support/xclock_gettime.c (xclock_gettime): New file. Provide
	clock_gettime wrapper for use in tests that fails the test rather
	than returning failure.

	* support/xtime.h: New file to declare xclock_gettime.

	* support/Makefile: Add xclock_gettime.c.

	* support/README: Mention xtime.h.
---
 ChangeLog                | 12 ++++++++++++
 support/Makefile         |  1 +
 support/README           |  1 +
 support/xclock_gettime.c | 29 +++++++++++++++++++++++++++++
 support/xtime.h          | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 76 insertions(+)
 create mode 100644 support/xclock_gettime.c
 create mode 100644 support/xtime.h
Adhemerval Zanella Netto - April 23, 2019, 1:59 p.m.
On 07/04/2019 16:33, Mike Crowe wrote:
> * support/xclock_gettime.c (xclock_gettime): New file. Provide
> 	clock_gettime wrapper for use in tests that fails the test rather
> 	than returning failure.
> 
> 	* support/xtime.h: New file to declare xclock_gettime.
> 
> 	* support/Makefile: Add xclock_gettime.c.
> 
> 	* support/README: Mention xtime.h.

LGTM with the change below.

> ---
>  ChangeLog                | 12 ++++++++++++
>  support/Makefile         |  1 +
>  support/README           |  1 +
>  support/xclock_gettime.c | 29 +++++++++++++++++++++++++++++
>  support/xtime.h          | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 76 insertions(+)
>  create mode 100644 support/xclock_gettime.c
>  create mode 100644 support/xtime.h
> 
> diff --git a/ChangeLog b/ChangeLog
> index 5ad8875..8c26806 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,15 @@
> +2019-04-06  Mike Crowe  <mac@mcrowe.com>
> +
> +	* support/xclock_gettime.c (xclock_gettime): New file. Provide
> +	clock_gettime wrapper for use in tests that fails the test rather
> +	than returning failure.
> +
> +	* support/xtime.h: New file to declare xclock_gettime.
> +
> +	* support/Makefile: Add xclock_gettime.c.
> +
> +	* support/README: Mention xtime.h.
> +
>  2019-04-05  Anton Youdkevitch <anton.youdkevitch@bell-sw.com>
>  
>  	* sysdeps/aarch64/multiarch/memcpy_thunderx2.S: Cleanup branching
> diff --git a/support/Makefile b/support/Makefile
> index f173565..1d37f70 100644
> --- a/support/Makefile
> +++ b/support/Makefile
> @@ -77,6 +77,7 @@ libsupport-routines = \
>    xbind \
>    xcalloc \
>    xchroot \
> +  xclock_gettime \
>    xclose \
>    xconnect \
>    xcopy_file_range \

Ok.

> diff --git a/support/README b/support/README
> index 476cfcd..d82f472 100644
> --- a/support/README
> +++ b/support/README
> @@ -10,6 +10,7 @@ error.  They are declared in these header files:
>  * support.h
>  * xsignal.h
>  * xthread.h
> +* xtime.h
>  
>  In general, new wrappers should be added to support.h if possible.
>  However, support.h must remain fully compatible with C90 and therefore

Ok.

> diff --git a/support/xclock_gettime.c b/support/xclock_gettime.c
> new file mode 100644
> index 0000000..91464fe
> --- /dev/null
> +++ b/support/xclock_gettime.c
> @@ -0,0 +1,29 @@
> +/* clock_gettime with error checking.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <support/xtime.h>
> +#include <support/xthread.h>
> +
> +
> +void
> +xclock_gettime (clockid_t clockid,
> +                struct timespec *ts)
> +{
> +  xpthread_check_return
> +    ("clock_gettime", clock_gettime (clockid, ts));
> +}

xpthread_check_return uses the returned values as errno, while clock_gettime sets
errno appropriately.  Just use other functions that set errno:

  int ret = clock_gettime (clockid, ts);
  if (ret < 0)
    FAIL_EXIT1 ("clock_gettime (\"%d\", {\"%ld\", \"%ld\"}): %m",
		clockid, (long int) ts.tv_sec, ts.tv_nsec);
  return ret

> diff --git a/support/xtime.h b/support/xtime.h
> new file mode 100644
> index 0000000..68af1a5
> --- /dev/null
> +++ b/support/xtime.h
> @@ -0,0 +1,33 @@
> +/* Support functionality for using time.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef SUPPORT_TIME_H
> +#define SUPPORT_TIME_H
> +
> +#include <time.h>
> +
> +__BEGIN_DECLS
> +
> +/* The following functions call the corresponding libc functions and
> +   terminate the process on error.  */
> +
> +void xclock_gettime (clockid_t clock, struct timespec *ts);
> +
> +__END_DECLS
> +
> +#endif /* SUPPORT_TIME_H */
> 

Ok.
Mike Crowe - April 25, 2019, 12:21 p.m.
On Tuesday 23 April 2019 at 10:59:00 -0300, Adhemerval Zanella wrote:
> > diff --git a/support/xclock_gettime.c b/support/xclock_gettime.c
> > new file mode 100644
> > index 0000000..91464fe
> > --- /dev/null
> > +++ b/support/xclock_gettime.c
> > @@ -0,0 +1,29 @@
> > +/* clock_gettime with error checking.
> > +   Copyright (C) 2019 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +#include <support/xtime.h>
> > +#include <support/xthread.h>
> > +
> > +
> > +void
> > +xclock_gettime (clockid_t clockid,
> > +                struct timespec *ts)
> > +{
> > +  xpthread_check_return
> > +    ("clock_gettime", clock_gettime (clockid, ts));
> > +}
> 
> xpthread_check_return uses the returned values as errno, while clock_gettime sets
> errno appropriately.  Just use other functions that set errno:
> 
>   int ret = clock_gettime (clockid, ts);
>   if (ret < 0)
>     FAIL_EXIT1 ("clock_gettime (\"%d\", {\"%ld\", \"%ld\"}): %m",
> 		clockid, (long int) ts.tv_sec, ts.tv_nsec);
>   return ret

Is it really worth outputting ts.tv_sec and ts.tv_nsec? If the call has
failed then they will just contain whatever was in them before the call -
which probably means uninitialised. Even worse, if clock_gettime fails with
EFAULT then attempting to read from ts will fault too.

I think I'd prefer just:

 FAIL_EXIT1 ("clock_gettime (%d): %m", clockid);

Are you happy with that?

Mike.
Adhemerval Zanella Netto - April 25, 2019, 12:30 p.m.
On 25/04/2019 09:21, Mike Crowe wrote:
> On Tuesday 23 April 2019 at 10:59:00 -0300, Adhemerval Zanella wrote:
>>> diff --git a/support/xclock_gettime.c b/support/xclock_gettime.c
>>> new file mode 100644
>>> index 0000000..91464fe
>>> --- /dev/null
>>> +++ b/support/xclock_gettime.c
>>> @@ -0,0 +1,29 @@
>>> +/* clock_gettime with error checking.
>>> +   Copyright (C) 2019 Free Software Foundation, Inc.
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library; if not, see
>>> +   <http://www.gnu.org/licenses/>.  */
>>> +
>>> +#include <support/xtime.h>
>>> +#include <support/xthread.h>
>>> +
>>> +
>>> +void
>>> +xclock_gettime (clockid_t clockid,
>>> +                struct timespec *ts)
>>> +{
>>> +  xpthread_check_return
>>> +    ("clock_gettime", clock_gettime (clockid, ts));
>>> +}
>>
>> xpthread_check_return uses the returned values as errno, while clock_gettime sets
>> errno appropriately.  Just use other functions that set errno:
>>
>>   int ret = clock_gettime (clockid, ts);
>>   if (ret < 0)
>>     FAIL_EXIT1 ("clock_gettime (\"%d\", {\"%ld\", \"%ld\"}): %m",
>> 		clockid, (long int) ts.tv_sec, ts.tv_nsec);
>>   return ret
> 
> Is it really worth outputting ts.tv_sec and ts.tv_nsec? If the call has
> failed then they will just contain whatever was in them before the call -
> which probably means uninitialised. Even worse, if clock_gettime fails with
> EFAULT then attempting to read from ts will fault too.

Not really, it was just a suggestion in fact.

> 
> I think I'd prefer just:
> 
>  FAIL_EXIT1 ("clock_gettime (%d): %m", clockid);
> 
> Are you happy with that?

LGTM, thanks.

Patch

diff --git a/ChangeLog b/ChangeLog
index 5ad8875..8c26806 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@ 
+2019-04-06  Mike Crowe  <mac@mcrowe.com>
+
+	* support/xclock_gettime.c (xclock_gettime): New file. Provide
+	clock_gettime wrapper for use in tests that fails the test rather
+	than returning failure.
+
+	* support/xtime.h: New file to declare xclock_gettime.
+
+	* support/Makefile: Add xclock_gettime.c.
+
+	* support/README: Mention xtime.h.
+
 2019-04-05  Anton Youdkevitch <anton.youdkevitch@bell-sw.com>
 
 	* sysdeps/aarch64/multiarch/memcpy_thunderx2.S: Cleanup branching
diff --git a/support/Makefile b/support/Makefile
index f173565..1d37f70 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -77,6 +77,7 @@  libsupport-routines = \
   xbind \
   xcalloc \
   xchroot \
+  xclock_gettime \
   xclose \
   xconnect \
   xcopy_file_range \
diff --git a/support/README b/support/README
index 476cfcd..d82f472 100644
--- a/support/README
+++ b/support/README
@@ -10,6 +10,7 @@  error.  They are declared in these header files:
 * support.h
 * xsignal.h
 * xthread.h
+* xtime.h
 
 In general, new wrappers should be added to support.h if possible.
 However, support.h must remain fully compatible with C90 and therefore
diff --git a/support/xclock_gettime.c b/support/xclock_gettime.c
new file mode 100644
index 0000000..91464fe
--- /dev/null
+++ b/support/xclock_gettime.c
@@ -0,0 +1,29 @@ 
+/* clock_gettime with error checking.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <support/xtime.h>
+#include <support/xthread.h>
+
+
+void
+xclock_gettime (clockid_t clockid,
+                struct timespec *ts)
+{
+  xpthread_check_return
+    ("clock_gettime", clock_gettime (clockid, ts));
+}
diff --git a/support/xtime.h b/support/xtime.h
new file mode 100644
index 0000000..68af1a5
--- /dev/null
+++ b/support/xtime.h
@@ -0,0 +1,33 @@ 
+/* Support functionality for using time.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef SUPPORT_TIME_H
+#define SUPPORT_TIME_H
+
+#include <time.h>
+
+__BEGIN_DECLS
+
+/* The following functions call the corresponding libc functions and
+   terminate the process on error.  */
+
+void xclock_gettime (clockid_t clock, struct timespec *ts);
+
+__END_DECLS
+
+#endif /* SUPPORT_TIME_H */