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! _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
