On (03/09/13 00:25), Jakub Hrozek wrote:
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:
ehlo,
It would be great to have enabled printf format string checking in RHEL7. Therefore I decided to send pateches for ticket https://fedorahosted.org/sssd/ticket/1945
Patch 0001 -- Even if this patch is first, it should be applied in upstream as 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 posix as a signed type, therefore format should be used "zd" Patch 0008 -- size_t is defined in ANSI c99 as unzigned type -> "zu"
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 type.
Problem is that sizeof(size_t) is different on 32-bit platform and 64-bit platform
OK
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, rlim_t...) and for some types I created macros in sss_format.h Patch 0010 -- key_serial_t is typedef of int32_t, but it could be defined differently in another (platforms/ implemantations of kerberos) Patch 0011 -- it seems that rlim_t us the same as uint64_t, but it was defined 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)
[snip]
Subject: [PATCH 10/16] Fix formating of variables with type: key_serial_t 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
ACK
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.
LS
Sorry the review has stalled for a bit. I think that once we resolve the uid_t patch, we can push the whole patchset.
I added 3 new macros for formating [ug]?id types: PRIid, PRIgid, PRIuid
BTW: I rebased patches and spot 3 new warnings :-)
LS