On Wed, Sep 11, 2013 at 07:37:29PM +0200, Jakub Hrozek wrote:
On Wed, Sep 11, 2013 at 04:57:57PM +0200, Lukas Slebodnik wrote:
On (10/09/13 17:12), Jakub Hrozek wrote:
On Wed, Sep 04, 2013 at 06:34:52PM +0200, Lukas Slebodnik wrote:
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
I like this approach. I just wonder if we should namespace these to say SPRI instead? Would it be too much work or doable with a search-and-replace?
BTW: I rebased patches and spot 3 new warnings :-)
Yeah, we really need to push these patches..
index efaa247dd22d2ae100e8881c010dfe26a9a10cca..5f2465ca6d8aef7fc486a033e7d3759050c0dec8 100644 --- a/src/providers/ldap/sdap_async_initgroups_ad.c +++ b/src/providers/ldap/sdap_async_initgroups_ad.c @@ -472,7 +472,7 @@ sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req *subreq) }
DEBUG(SSSDBG_TRACE_LIBS,
("Processing membership GID [%lu]\n",
("Processing membership GID [%u]\n", gid));This change is overwritten in a later patch.
I couldn't apply patches 7 and 16, can you rebase?
Attached patches are rebased on top of current master.
All macros from sss_format.h were renamed from PRI -> SPRI
LS
These patches work for me.
ACK
Pushed all to master. Great work!