Re: [PATCH v3 1/2] libuuid: Refactor UUID time conversion for pre-epoch dates

Hi Kiran,

On 2025-12-18 16:31:21-0500, Kiran Rangoon wrote:
> I revised the code in response to your feedback.

Thanks!

> > Looking at this more closely: __uuid_time() is directly exposed to users
> > of libuuid as uuid_time(). This means that any change of the function's
> > contract would break all external users and should be avoided at all
> > costs. 
> 
> Reversed the change there.

Your new revision is based on top of your old series.
Instead it should always be based on upstream code.
In this case the util-linux master branch.

> > Fortunately it turns out that the timestamps embedded in UUIDs
> > only use 60 bits. This means that the calculation can be performed in an
> > int64_t without any risk of over- or underflow. Please try to implement
> > it that way instead. 
> 
> I'm using signed int64_t here as suggested.
> 
> > It might be useful to change the signature of gregorian_to_unix() to
> > "static void gregorian_to_unix(uint64_t ts, struct timeval *tv)".
> 
> I revised the code to use a struct timeval now. 

That looks good. However that refactoring should be in its own commit.
More on that below.

> > Also please perform each logical step in a dedicated commit.
> 
> I made two commits, one for the code change and the other for tests, but
> if there is a way you would perfer me to do it I could change it.

The test is a bad candidate to split out. After each commit the
repository needs to be in a consistent state, which means that the code
builds and the tests pass. As the code change breaks the existing test,
the test needs to be adapted in lockstep.

I see the following commits:

1) Switch to the simpler constant definition
2) Pass 'struct timeval' to gregorian_to_unix()
3) Fix the overflow, adapt the existing test
4) Add a hand full of additional testcases.

Also this patch now lost its useful commit message which got replaced by
our discussion. This should not happen. Each commit should explain why
it is being done.

> ---
>  libuuid/src/uuid_time.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/libuuid/src/uuid_time.c b/libuuid/src/uuid_time.c
> index f0d2c8f36..293fc7e68 100644
> --- a/libuuid/src/uuid_time.c
> +++ b/libuuid/src/uuid_time.c
> @@ -60,34 +60,29 @@
>  /* prototype to make compiler happy */
>  time_t __uuid_time(const uuid_t uu, struct timeval *ret_tv);
>  
> -static int64_t gregorian_to_unix(uint64_t ts)
> +static void gregorian_to_unix(uint64_t ts, struct timeval *tv)
>  {
> -    const uint64_t offset = 0x01B21DD213814000ULL;
> +	const uint64_t offset = 0x01B21DD213814000ULL;
> +	int64_t t = (int64_t) ts - (int64_t) offset;

Whitespace corruption?

> -    if (ts < offset) {
> -        errno = EOVERFLOW;
> -        return -1;
> -    }
> -
> -    return ts - offset;
> +	tv->tv_sec = t / 10000000;
> +	tv->tv_usec = (t % 10000000) / 10;
>  }

(...)


Thomas