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.
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" 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) Patch 0016 -- trivial
LS
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.
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" 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) Patch 0016 -- trivial
LS
Hi,
I'm looking at the patches now, and I see, that somewhere you changed the debug levels to the new style and somewhere you left the numeric value. Could you please replace the debug levels with new style anywhere you're modifying code?
Ondra
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.
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" 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) Patch 0016 -- trivial
LS
Hi,
I'm looking at the patches now, and I see, that somewhere you changed the debug levels to the new style and somewhere you left the numeric value. Could you please replace the debug levels with new style anywhere you're modifying code?
Ondra
Thank you for review. I didn't fix warnings at once, because there were a lot of warnings 180+. So it seems, that I changed debug levels only sometimes.
Updated patches are attached.
LS
On 07/24/2013 10:32 AM, 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.
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" 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) Patch 0016 -- trivial
LS
Hi,
I'm looking at the patches now, and I see, that somewhere you changed the debug levels to the new style and somewhere you left the numeric value. Could you please replace the debug levels with new style anywhere you're modifying code?
Ondra
Thank you for review. I didn't fix warnings at once, because there were a lot of warnings 180+. So it seems, that I changed debug levels only sometimes.
Updated patches are attached.
LS
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Ack to all patches
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..
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 :)
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 then defines __UID_T_TYPE to __uid_t which in turn gets typedefed to uid_t)
Patch 0016 -- trivial
LS
Hi,
I'm looking at the patches now, and I see, that somewhere you changed the debug levels to the new style and somewhere you left the numeric value. Could you please replace the debug levels with new style anywhere you're modifying code?
Ondra
Thank you for review. I didn't fix warnings at once, because there were a lot of warnings 180+. So it seems, that I changed debug levels only sometimes.
Updated patches are attached.
LS
From ee2cfd16d12e11d4878d8137b958e797bc2228a3 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Thu, 23 May 2013 13:14:37 +0200 Subject: [PATCH 01/16] Enable printf format string checking
https://fedorahosted.org/sssd/ticket/1945
configure.ac | 18 ++++++++++++++++++ src/util/util.h | 14 ++++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac index b9b72e3d7cd3adf1bd127e6ae9546a460ee0055e..abde94774fb3d4e60302ff74ad4e9829550cfcc7 100644 --- a/configure.ac +++ b/configure.ac @@ -264,6 +264,24 @@ if test x"$sss_client_cv_attribute_destructor" = xyes ; then [whether compiler supports __attribute__((destructor))]) fi
+AC_CACHE_CHECK([whether compiler supports __attribute__((format))],
sss_client_cv_attribute_format,[AC_COMPILE_IFELSE([AC_LANG_SOURCE([void debug_fn(const char *format, ...) __attribute__ ((format (printf, 1, 2)));])],[sss_client_cv_attribute_format=yes],
Can you remove the _client_ part? The variable is not used only in client..
[AC_MSG_RESULT([no])AC_MSG_WARN([compiler does NOT support __attribute__((format))])])])+if test x"$sss_client_cv_attribute_format" = xyes ; then
- AC_DEFINE(HAVE_FUNCTION_ATTRIBUTE_FORMAT, 1,
[whether compiler supports __attribute__((format))])+fi
PKG_CHECK_MODULES([CHECK], [check >= 0.9.5], [have_check=1], [have_check=]) if test x$have_check = x; then AC_MSG_WARN([Without the 'CHECK' libraries, you will be unable to run all tests in the 'make check' suite]) diff --git a/src/util/util.h b/src/util/util.h index 5acc67bec7ee94b85963d04f5bfcca017cf87abd..c328f7e9fb6d4bbd8667d171ccd0e593d1f0e70f 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -54,13 +54,19 @@
#define CLEAR_MC_FLAG "clear_mc_flag"
+#ifdef HAVE_FUNCTION_ATTRIBUTE_FORMAT +#define SSS_ATTRIBUTE_PRINTF(a1, a2) __attribute__ ((format (printf, a1, a2))) +#else +#define SSS_ATTRIBUTE_PRINTF(a1, a2) +#endif
extern const char *debug_prg_name; extern int debug_level; extern int debug_timestamps; extern int debug_microseconds; extern int debug_to_file; extern const char *debug_log_file; -void debug_fn(const char *format, ...); +void debug_fn(const char *format, ...) SSS_ATTRIBUTE_PRINTF(1, 2); int debug_get_level(int old_level); int debug_convert_old_level(int old_level); errno_t set_debug_file_from_fd(const int fd); @@ -128,7 +134,7 @@ errno_t set_debug_file_from_fd(const int fd); memcpy(__debug_macro_datetime, ctime(&__debug_macro_tv.tv_sec), 19); \ __debug_macro_datetime[19] = '\0'; \ if (debug_microseconds) { \
debug_fn("(%s:%.6d %d) [%s] [%s] (%#.4x): ", \
debug_fn("(%s:%.6ld %d) [%s] [%s] (%#.4x): ", \
This change seems correct to me, but can you split it to a separate patch? I'd like to have only the autoconf macro in the new patch.
__debug_macro_datetime, __debug_macro_tv.tv_usec, \ __debug_macro_year, debug_prg_name, \ __FUNCTION__, __debug_macro_newlevel); \@@ -169,7 +175,7 @@ errno_t set_debug_file_from_fd(const int fd); memcpy(__debug_macro_datetime, ctime(&__debug_macro_tv.tv_sec), 19); \ __debug_macro_datetime[19] = '\0'; \ if (debug_microseconds) { \
debug_fn("(%s:%.6d %d) [%s] [%s] (%#.4x): %s\n", \
debug_fn("(%s:%.6ld %d) [%s] [%s] (%#.4x): %s\n", \ __debug_macro_datetime, __debug_macro_tv.tv_usec, \ __debug_macro_year, debug_prg_name, \ function, __debug_macro_newlevel, message); \@@ -292,7 +298,7 @@ void talloc_log_fn(const char *msg); #define SSS_LOG_INFO 6 /* informational */ #define SSS_LOG_DEBUG 7 /* debug-level messages */
-void sss_log(int priority, const char *format, ...); +void sss_log(int priority, const char *format, ...) SSS_ATTRIBUTE_PRINTF(2, 3);
/* from server.c */ struct main_context { -- 1.8.3.1
Subject: [PATCH 02/16] Fix formating of variables with type: long Subject: [PATCH 03/16] Fix formating of variables with type: unsigned long Subject: [PATCH 04/16] Fix formating of variables with type: int Subject: [PATCH 05/16] Fix pointer formatting Subject: [PATCH 06/16] Use the same variable type like in struct
ACK, trivial patches
Subject: [PATCH 07/16] Fix formating of variables with type: ssize_t
ACK. I think it's quite standard practice to use zd for ssize_t, but I haven't found a canonical source in my C books either.
Subject: [PATCH 08/16] Fix formating of variables with type: size_t
ACK
Subject: [PATCH 09/16] Adding new header for printf formating macros
I wonder if it makes sense to simply add the new header to util.h instead of adding it to individual sources. It's only ever going to contain macros, right? But I agree with having the file separately from util.h it might be needed in cases where we don't want to include the whole util.h.
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?
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?
Subject: [PATCH 16/16] Fix warning: data argument not used by format string
Oof, these are bad. Thank you. ACK.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/21/2013 07:46 AM, Jakub Hrozek wrote:
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 think I disagree here. I'd rather see us using explicit values. It leaves less room for mistakes on different platforms.
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.
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
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
(and then defines __UID_T_TYPE to __uid_t which in turn gets typedefed to uid_t)
Patch 0016 -- trivial
LS
Hi,
I'm looking at the patches now, and I see, that somewhere you changed the debug levels to the new style and somewhere you left the numeric value. Could you please replace the debug levels with new style anywhere you're modifying code?
Ondra
Thank you for review. I didn't fix warnings at once, because there were a lot of warnings 180+. So it seems, that I changed debug levels only sometimes.
Updated patches are attached.
LS
From ee2cfd16d12e11d4878d8137b958e797bc2228a3 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Thu, 23 May 2013 13:14:37 +0200 Subject: [PATCH 01/16] Enable printf format string checking
https://fedorahosted.org/sssd/ticket/1945
configure.ac | 18 ++++++++++++++++++ src/util/util.h | 14 ++++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac index b9b72e3d7cd3adf1bd127e6ae9546a460ee0055e..abde94774fb3d4e60302ff74ad4e9829550cfcc7 100644 --- a/configure.ac +++ b/configure.ac @@ -264,6 +264,24 @@ if test x"$sss_client_cv_attribute_destructor" = xyes ; then [whether compiler supports __attribute__((destructor))]) fi
+AC_CACHE_CHECK([whether compiler supports __attribute__((format))],
sss_client_cv_attribute_format,[AC_COMPILE_IFELSE([AC_LANG_SOURCE([void debug_fn(const char *format, ...) __attribute__ ((format (printf, 1, 2)));])],[sss_client_cv_attribute_format=yes],Can you remove the _client_ part? The variable is not used only in client..
Fixed. (I inspired by attribute_destructor)
[AC_MSG_RESULT([no])AC_MSG_WARN([compiler does NOT support __attribute__((format))])])])+if test x"$sss_client_cv_attribute_format" = xyes ; then
- AC_DEFINE(HAVE_FUNCTION_ATTRIBUTE_FORMAT, 1,
[whether compiler supports __attribute__((format))])+fi
PKG_CHECK_MODULES([CHECK], [check >= 0.9.5], [have_check=1], [have_check=]) if test x$have_check = x; then AC_MSG_WARN([Without the 'CHECK' libraries, you will be unable to run all tests in the 'make check' suite]) diff --git a/src/util/util.h b/src/util/util.h index 5acc67bec7ee94b85963d04f5bfcca017cf87abd..c328f7e9fb6d4bbd8667d171ccd0e593d1f0e70f 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -54,13 +54,19 @@
#define CLEAR_MC_FLAG "clear_mc_flag"
+#ifdef HAVE_FUNCTION_ATTRIBUTE_FORMAT +#define SSS_ATTRIBUTE_PRINTF(a1, a2) __attribute__ ((format (printf, a1, a2))) +#else +#define SSS_ATTRIBUTE_PRINTF(a1, a2) +#endif
extern const char *debug_prg_name; extern int debug_level; extern int debug_timestamps; extern int debug_microseconds; extern int debug_to_file; extern const char *debug_log_file; -void debug_fn(const char *format, ...); +void debug_fn(const char *format, ...) SSS_ATTRIBUTE_PRINTF(1, 2); int debug_get_level(int old_level); int debug_convert_old_level(int old_level); errno_t set_debug_file_from_fd(const int fd); @@ -128,7 +134,7 @@ errno_t set_debug_file_from_fd(const int fd); memcpy(__debug_macro_datetime, ctime(&__debug_macro_tv.tv_sec), 19); \ __debug_macro_datetime[19] = '\0'; \ if (debug_microseconds) { \
debug_fn("(%s:%.6d %d) [%s] [%s] (%#.4x): ", \
debug_fn("(%s:%.6ld %d) [%s] [%s] (%#.4x): ", \This change seems correct to me, but can you split it to a separate patch? I'd like to have only the autoconf macro in the new patch.
It made a big noise in each DEBUG macro and I forgot about those lines. Moved to another patch.
__debug_macro_datetime, __debug_macro_tv.tv_usec, \ __debug_macro_year, debug_prg_name, \ __FUNCTION__, __debug_macro_newlevel); \@@ -169,7 +175,7 @@ errno_t set_debug_file_from_fd(const int fd); memcpy(__debug_macro_datetime, ctime(&__debug_macro_tv.tv_sec), 19); \ __debug_macro_datetime[19] = '\0'; \ if (debug_microseconds) { \
debug_fn("(%s:%.6d %d) [%s] [%s] (%#.4x): %s\n", \
debug_fn("(%s:%.6ld %d) [%s] [%s] (%#.4x): %s\n", \ __debug_macro_datetime, __debug_macro_tv.tv_usec, \ __debug_macro_year, debug_prg_name, \ function, __debug_macro_newlevel, message); \@@ -292,7 +298,7 @@ void talloc_log_fn(const char *msg); #define SSS_LOG_INFO 6 /* informational */ #define SSS_LOG_DEBUG 7 /* debug-level messages */
-void sss_log(int priority, const char *format, ...); +void sss_log(int priority, const char *format, ...) SSS_ATTRIBUTE_PRINTF(2, 3);
/* from server.c */ struct main_context { -- 1.8.3.1
Subject: [PATCH 02/16] Fix formating of variables with type: long Subject: [PATCH 03/16] Fix formating of variables with type: unsigned long Subject: [PATCH 04/16] Fix formating of variables with type: int Subject: [PATCH 05/16] Fix pointer formatting Subject: [PATCH 06/16] Use the same variable type like in struct
ACK, trivial patches
Subject: [PATCH 07/16] Fix formating of variables with type: ssize_t
ACK. I think it's quite standard practice to use zd for ssize_t, but I haven't found a canonical source in my C books either.
ssize_t is defined by POSIX.
Subject: [PATCH 08/16] Fix formating of variables with type: size_t
ACK
Subject: [PATCH 09/16] Adding new header for printf formating macros
I wonder if it makes sense to simply add the new header to util.h instead of adding it to individual sources. It's only ever going to contain macros, right? But I agree with having the file separately from util.h it might be needed in cases where we don't want to include the whole util.h.
I added include to util.h. I removed "#include util/sss_format.h" from another patches, but only in case if file had line "#include util/util.h"
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 ------------------
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
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.
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
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?
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
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
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@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Ondrej Kos -
Stephen Gallagher