On Wed, Aug 21, 2013 at 03:35:11PM +0200, Lukas Slebodnik wrote:
On (21/08/13 13:46), Jakub Hrozek wrote:
>Sorry it took so long before we got to another review of the patch. The
>ticket is targeted for 1.12 so initially I was thinking of the patchset
>as lower priority than the other work.
>On Wed, Jul 24, 2013 at 10:32:29AM +0200, Lukas Slebodnik wrote:
>> On (24/07/13 09:41), Ondrej Kos wrote:
>> >On 07/23/2013 06:21 PM, Lukas Slebodnik wrote:
>> >>It would be great to have enabled printf format string checking in
>> >>Therefore I decided to send pateches for ticket
>> >>Patch 0001 -- Even if this patch is first, it should be applied in
>> >> last.
> ^^^^ Does it matter? I'll apply the whole set anyway..
It can make a big noise in case of bisecting.
I moved this patch to the end.
Thanks. I'll try to remember to ask someone to double-check before pushing
>Some comments below..
>> >>Patch 0002 - 0006 -- trivial
>> >>Patch 0007 -- ssize_t is not defined in ANSI c99, IIRC it is defined in
>> >> as a signed type, therefore format should be used
>> >>Patch 0008 -- size_t is defined in ANSI c99 as unzigned type ->
>btw size_t seems to be part of standard library, not the language
>specification per se. Not that it matters for the patch :)
size_t can be part of standard library, but ANSI c99 says size_t is unsigned
Problem is that sizeof(size_t) is different on 32-bit platform and 64-bit
>> >>Patch 0009 -- wrapper for inttypes.h and for future format macros.
>> >>Patch 0012 -- formating types defined in stdint.h uint_32_t ...
>> >> -- there are used macros defined in inttypes.h
>> >>Patches 0010 - 0015 fix formating for special variables (key_serial_t,
>> >> and for some types I created macros in
>> >>Patch 0010 -- key_serial_t is typedef of int32_t, but it could be
>> >> differently in another (platforms/ implemantations of
>> >>Patch 0011 -- it seems that rlim_t us the same as uint64_t, but it was
>> >> using conditional build an not a typedef of std types.
>> >>Patch 0013 -- time_t is defined as "long int"
sizeof_i386(time_t) != sizeof_x86_64(time_t)
>> >>Patch 0014 -- ber_int and ber_tag are typedef for int and unsigned long
>> >>Patch 0015 -- gid_t and uid_t are typedef as unsigned
>> >> (I checked linux 32 bit, linu 64_bit and freebsd 64 bit)
>Actually /usr/include/bits/typesizes.h says:
>#define __UID_T_TYPE __U32_TYPE
and you forgot to add:
shell$ grep -RniI __U32_TYPE
bits/types.h:101:#define __U32_TYPE unsigned int
I think this is lucky coincidence where unsigned int is 32bits wide.
But to be honest I haven't found any information that would state that
uid_t or gid_t is guaranteed to be 32bit (even though some places in the
SSSD rely on uid_t == uint32_t). As far as I can see, POSIX simply says
that both are opaque numeric types. Too bad the system headers don't
provide some kind of format specifier..
I wonder if we should check the width during configure and define our
own specifier based on what we discover?
>(and then defines __UID_T_TYPE to __uid_t which in turn gets typedefed to uid_t)
>> Subject: [PATCH 10/16] Fix formating of variables with type:
>> Subject: [PATCH 11/16] Fix formating of variables with type: rlim_t
>> Subject: [PATCH 12/16] Fix formating of variables with type defined in stdint.h
>Nice, I didn't know these existed. But to be honest, these format specifiers
>seem ugly to me..wouldn't it be nicer to cast the variables to a larger type
>(long or unsigned long) and then use plan C conversion?
I agree with Stephen casting is not very portable.
Some platforms needn't have defned long long unsigend or there can be another
type which is bigger.
uintmax_t/intmax_t could be used as the larget type.
In this case modifier "j" should be used
format_fun("%jd %ju\n" (uintmax_t) uint_8_var, (intmax_t)int8_t_var)
but it is overkill to cast uint_8_var to 64_vit variable (or in future 128 bit:-)
And last point. I don't like casting of argumets in formating functions.
"man inttypes.h" is very helpful
OK, I'm convinced :-)
>> Subject: [PATCH 13/16] Fix formating of variables with type: time_t
>I think this is OK, my C reference says that time_t can be of "any arithmetic
>type" but also states that long is traditionally used.
>> Subject: [PATCH 14/16] Fix formating of variables with ber_ type
>> Subject: [PATCH 15/16] Fix formating of variables with type: [gu]?id_t
>This is the main point of discussion in the patchset I think. IIRC POSIX
>states that IDs are 32bit unsigned integers. That's precisely why we have
>casted them up to unsigned (long) long. Did you see warnings with some
>strict combination of CFLAGS?
-Wformat is automatically enabled by -Wall, There is no problem with CFLAGS.
You can try to revert this patch (or do not apply this patch)
And warnings will appear. clang gives more hints.
But it is possibel to add another macro to header file sss_format.h
>> Subject: [PATCH 16/16] Fix warning: data argument not used by format string
>Oof, these are bad. Thank you. ACK.
Those fixes are not critical, only unsufficient information will be printed.
Updated patches are attached.
Sorry the review has stalled for a bit. I think that once we resolve the
uid_t patch, we can push the whole patchset.