Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

From: Tomoyoshi ASANO <asa@lineo.co.jp>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Deepa Dinamani <deepa.kernel@gmail.com>,
	y2038@lists.linaro.org, john.stultz@linaro.org,
	linux-kernel@vger.kernel.org, arnd@arndb.de
Subject: Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64
Date: Tue, 21 Mar 2017 06:40:06 +0900	[thread overview]
Message-ID: <20170321064005.D608.22C34B8C@lineo.co.jp> (raw)
In-Reply-To: <alpine.DEB.2.20.1703191133270.3781@nanos>


On Sun, 19 Mar 2017 11:51:34 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Sat, 18 Mar 2017, Deepa Dinamani wrote:
> 
> >  struct timespec is not y2038 safe.
> >  Replace the posix_clock ops interfaces to use
> >  struct timespec64.
> >  The patch also changes struct itimerspec interfaces to
> >  struct itimerspec64 as itimerspec internally uses timespec
> >  and itimerspec64 uses timespec64.
> >  PTP clocks is the only module that sets up these interfaces.
> >  All individual drivers rely on PTP class driver for exposure
> >  to userspace. Hence, the change also deals with fixing up these
> >  PTP interfaces.
> >  The patch also changes dynamic posix clock implementation to
> >  reflect the changes in the functional clock interface.
> 
> Please do not explain WHAT the patch is doing. We can see that from the
> diff itself. What's important is the WHY. A good changelog is structured in
> paragraphs, which explain the context, the problem and the solution. Please
> read Documentation/process/submitting-patches.rst. Let me give you an
> example.
> 
>   struct timespec is not Y2038 safe on 32 bit machines and needs to be
>   replaced with struct timespec64.
> 
>   The posix clock functions use struct timespec directly and through struct
>   itimerspec.
> 
>   Change all function prototypes to use timespec64 and itimerspec64 and fix
>   up all implementations.
> 
> That gives all the information a reviewer or someone who is looking at the
> commit later needs: context, problem scope and solution.
> 
> Hmm?
> 
> >  /* posix clock implementation */
> >  
> > -static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp)
> > +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp)
> 
> That's a pretty pointless exercise. getres() returns the resolution of the
> clock which obviously can never be affected by Y2038. 
> 
> >  static int pc_clock_settime(clockid_t id, const struct timespec *ts)
> >  {
> >  	struct posix_clock_desc cd;
> > +	struct timespec64 ts64 = timespec_to_timespec64(*ts);
> >  	int err;
> 
> Please order the variables as a reverse fir tree sorted by length.
> 
> 	struct timespec64 ts64 = timespec_to_timespec64(*ts);
>  	struct posix_clock_desc cd;
>  	int err;
> 
> That's way simpler to parse than the above random length odering.
> 
> > @@ -418,14 +427,19 @@ static int pc_timer_settime(struct k_itimer *kit, int flags,
> >  {
> >  	clockid_t id = kit->it_clock;
> >  	struct posix_clock_desc cd;
> > +	struct itimerspec64 old64;
> > +	struct itimerspec64 ts64 = itimerspec_to_itimerspec64(ts);
> 
> See above.
> 
> Thanks,
> 
> 	tglx
> _______________________________________________
> Y2038 mailing list
> Y2038@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/y2038

--
 　┏━━━━━━━━━━━━━━━━━━━━┓
 ┏┫  リネオは お客様の これりな を後押し   ┣┓
 ┃┗┳━━━━━━━━━━━━━━━━━━┳┛┃
 ┗━┛         http://www.lineo.co.jp     ┗━┛