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