lkml.org 
[lkml]   [2014]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH v2 02/11] time: Add mktime64() safe version(using time64_t)
    On Thu, 30 Oct 2014, pang.xunlei wrote:

    Same problem with the $subject as in the previous patch.

    > As part of addressing 2038 saftey for in-kernel uses, this patch

    s/saftey/safety/

    saftey is something completely different.

    > +/*
    > + * Converts Gregorian date to seconds since 1970-01-01 00:00:00.
    > + * Assumes input in normal date format, i.e. 1980-12-31 23:59:59
    > + * => year=1980, mon=12, day=31, hour=23, min=59, sec=59.
    > + *
    > + * [For the Julian calendar (which was used in Russia before 1917,
    > + * Britain & colonies before 1752, anywhere else before 1582,
    > + * and is still in use by some communities) leave out the
    > + * -year/100+year/400 terms, and add 10.]
    > + *
    > + * This algorithm was first published by Gauss (I think).
    > + *

    So this blindly copies the comment from mktime but misses to add a
    proper docbook comment for the function.

    > + * Safe version 2038 safety on 32-bit systems.

    This sentence makes no sense at all. Aside of that you want to do the
    same as I suggested for do_settimeofday64

    * mktime64 - Convert gregrorian date to seconds since 1970. y2038 safe

    > + */
    > +time64_t
    > +mktime64(const unsigned int year0, const unsigned int mon0,
    > + const unsigned int day, const unsigned int hour,
    > + const unsigned int min, const unsigned int sec)

    Make this

    time64_t mktime64(const unsigned int year0, const unsigned int mon0,
    const unsigned int day, const unsigned int hour,
    const unsigned int min, const unsigned int sec)

    please.

    > +{
    > + time64_t ret;
    > + unsigned int mon = mon0, year = year0;

    I pretty much prefer the following style:

    + unsigned int mon = mon0, year = year0;
    + time64_t ret;

    Way simpler to parse.


    > + /* 1..12 -> 11,12,1..10 */
    > + if (0 >= (int) (mon -= 2)) {
    > + mon += 12; /* Puts Feb last since it has leap day */

    Please get rid of the tail comments while you are at it:

    /* Put Feb last since it has a leap day */
    mon += 12;

    Again simpler to parse.

    > + year -= 1;
    > + }
    > +
    > + ret = (year/4 - year/100 + year/400 + 367*mon/12 + day) + year*365 - 719499;

    For readability sake

    > + ret = ret*24 + hour; /* now have hours */

    Please kill these pointless comments. The calculation above is the one
    which could do with a comment not the obvious days to hrs and hrs to
    min and min to sec conversions.

    We do not blindly copy code and leave the mess unchanged.

    > + ret = ret*60 + min; /* now have minutes */
    > + ret = ret*60 + sec; /* finally seconds */
    > +
    > + return ret;
    > +}
    > +
    > +EXPORT_SYMBOL(mktime64);

    EXPORT_SYMBOL_GPL please.

    > +
    > /* Converts Gregorian date to seconds since 1970-01-01 00:00:00.
    > * Assumes input in normal date format, i.e. 1980-12-31 23:59:59
    > * => year=1980, mon=12, day=31, hour=23, min=59, sec=59.
    > @@ -318,6 +356,7 @@ EXPORT_SYMBOL(timespec_trunc);
    > * WARNING: this function will overflow on 2106-02-07 06:28:16 on
    > * machines where long is 32-bit! (However, as time_t is signed, we
    > * will already get problems at other places on 2038-01-19 03:14:08)
    > + * TODO: [2038 safety] should be replaced by mktime64().
    > */
    > unsigned long
    > mktime(const unsigned int year0, const unsigned int mon0,

    Why are you keeping this implementation instead of replacing the
    comment by a simple one liner

    /*
    * mktime - Convert gregrorian date to seconds since 1970. Deprecated. Use mktime64
    */

    No need to add a full docbook comment as this is going away anyway.

    and change the implementation to
    {
    return mktime64(....);
    }
    Thanks,

    tglx


    \
     
     \ /
      Last update: 2014-10-30 15:01    [W:4.138 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site