lib/util:tests/time.c - "test_timestring" - fix it on Solaris

Matthias Dieter Wallnöfer mdw at samba.org
Wed Dec 22 03:40:22 MST 2010


As far as I've seen it's used by (debug) messages and comments. And the 
output is never reworked further. So are you okay if I merge?

Cheers,
Matthias

Jelmer Vernooij wrote:
> Hi Matthias,
>
> On Wed, 2010-12-22 at 10:13 +0100, Matthias Dieter Wallnöfer wrote:
>    
>> another patch proposal. This simply specifies more precisely the output
>> of the date and it works on both Solaris and Linux (and I bet on other
>> Unix systems as well). In my eyes this should be definitely okay for
>> passing "samba4.local.time".
>>      
> That seems reasonable to me. Have you checked how strict the callers of
> timestring() rely on its format?
>
> Cheers,
>
> Jelmer
>
>    
>>> diff --git a/lib/util/time.c b/lib/util/time.c
>>> index a1f2b08..770ebc4 100644
>>> --- a/lib/util/time.c
>>> +++ b/lib/util/time.c
>>> @@ -387,7 +387,7 @@ _PUBLIC_ char *timestring(TALLOC_CTX *mem_ctx,
>>> time_t t)
>>>          /* Some versions of gcc complain about using some special format
>>>           * specifiers. This is a bug in gcc, not a bug in this code. See a
>>>           * recent strftime() manual page for details. */
>>> -       strftime(tempTime,sizeof(tempTime)-1,"%c %Z",tm);
>>> +       strftime(tempTime,sizeof(tempTime)-1,"%a %b %e %X %Y %Z",tm);
>>>          TimeBuf = talloc_strdup(mem_ctx, tempTime);
>>>   #else
>>>          TimeBuf = talloc_strdup(mem_ctx, asctime(tm));
>>>        
>> Cheers,
>> Matthias
>>
>> Matthias Dieter Wallnöfer wrote:
>>      
>>> Okay, I thought such a little difference (the "0" or not) wouldn't
>>> matter - but I will revert.
>>>
>>> Matthias
>>>
>>> Jelmer Vernooij wrote:
>>>        
>>>> On Mon, 2010-12-06 at 11:38 +0300, Matthieu Patou wrote:
>>>>          
>>>>> I guess you have to introduce a test in  lib/util/wscript_configure to
>>>>> define
>>>>> HAVE_CORRECT_STRFTIME by testing the output of strftime() and if the
>>>>> output is not ok then we should use asctime (at least on sun8 it
>>>>> seems ok).
>>>>>            
>>>> I think we should rather try to keep all platform-specific workarounds
>>>> in lib/replace. So if the strftime() doesn't do what we expect it to I
>>>> think we should add a strftime() replacement to lib/replace.
>>>>
>>>> Cheers,
>>>>
>>>> Jelmer
>>>>
>>>>          
>>>>> Have a look at changeset 4ea7d4694a8353fc55ecd12cb09b9c91ffde7b3f
>>>>> On 06/12/2010 05:59, Andrew Bartlett wrote:
>>>>>
>>>>> Matthieu.
>>>>>            
>>>>>> On Sun, 2010-12-05 at 23:10 +0100, Matthias Dieter Wallnöfer wrote:
>>>>>>              
>>>>>>> The branch, master has been updated
>>>>>>>           via  654e010 lib/util:tests/time.c - "test_timestring" -
>>>>>>> fix it on Solaris
>>>>>>>          from  55dba7b s4:cluster/cluster.h - fix another gcc 3.4
>>>>>>> "struct" warning
>>>>>>>
>>>>>>> http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
>>>>>>>
>>>>>>>
>>>>>>> - Log
>>>>>>> -----------------------------------------------------------------
>>>>>>> commit 654e0102ddb0acaaf45fb55c15818722235fcc9f
>>>>>>> Author: Matthias Dieter Wallnöfer<mdw at samba.org>
>>>>>>> Date:   Sun Dec 5 22:20:06 2010 +0100
>>>>>>>
>>>>>>>        lib/util:tests/time.c - "test_timestring" - fix it on Solaris
>>>>>>>
>>>>>>>        Solaris returns "Thu Jan 01" and not "Thu Jan  1" -
>>>>>>> therefore proof for
>>>>>>>        both.
>>>>>>>                
>>>>>> I'm not sure that this patch is correct.
>>>>>>
>>>>>> Given we had a test for a particular value, and we carefully build the
>>>>>> value with strftime() if available, why does this patch accept
>>>>>> different
>>>>>> values, rather than fix the the timestring() function?
>>>>>>
>>>>>> In general, I'm suspicious of patches that fix tests by changing the
>>>>>> test result rather than fixing the original function.
>>>>>>
>>>>>> Perhaps none of the callers care, as long as it's clear to human
>>>>>> readers
>>>>>> of the string, but there are rather a lot of callers to this function.
>>>>>>
>>>>>> Andrew Bartlett
>>>>>>
>>>>>>              
>>>>          
>>>
>>>        
>>      
>    



More information about the samba-technical mailing list