| Message ID | 2285954c62e0401291aa3f5055bc79c6789149d7.1303115468.git.ext-andriy.shevchenko@nokia.com |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 18, 2011 at 04:31, Andy Shevchenko wrote: > The casting of __off64_t to unsigned long potentially wrong for values higher > than ULONG_MAX. Let's fix that. i dont think this is the way to go. on 64bit systems, long long is 128bits. i imagine the way to go (assuming we're always using LFS) is to use PRIu64 from inttypes.h printf("\td %04o %9" PRIu64 " %5d:%-3d %s\n", e->sb.st_mode & ~S_IFMT, (unsigned long long) e->sb.st_size, (int) (e->sb.st_uid), (int) (e->sb.st_gid), e->name); -mike
On Mon, 2011-04-18 at 09:49 -0400, Mike Frysinger wrote: > On Mon, Apr 18, 2011 at 04:31, Andy Shevchenko wrote: > > The casting of __off64_t to unsigned long potentially wrong for values higher > > than ULONG_MAX. Let's fix that. > > i dont think this is the way to go. on 64bit systems, long long is > 128bits. i imagine the way to go (assuming we're always using LFS) is > to use PRIu64 from inttypes.h sizeof(unsigned long long) is 8 (64 bits) on my x86_64 fedora.
On Mon, Apr 18, 2011 at 09:55, Artem Bityutskiy wrote: > On Mon, 2011-04-18 at 09:49 -0400, Mike Frysinger wrote: >> On Mon, Apr 18, 2011 at 04:31, Andy Shevchenko wrote: >> > The casting of __off64_t to unsigned long potentially wrong for values higher >> > than ULONG_MAX. Let's fix that. >> >> i dont think this is the way to go. on 64bit systems, long long is >> 128bits. i imagine the way to go (assuming we're always using LFS) is >> to use PRIu64 from inttypes.h > > sizeof(unsigned long long) is 8 (64 bits) on my x86_64 fedora. so it is. i still think PRIu64 is the correct way to handle this as there is no sizeof() assumption and no need for casting. -mike
On Mon, 2011-04-18 at 10:04 -0400, Mike Frysinger wrote: > On Mon, Apr 18, 2011 at 09:55, Artem Bityutskiy wrote: > > On Mon, 2011-04-18 at 09:49 -0400, Mike Frysinger wrote: > >> On Mon, Apr 18, 2011 at 04:31, Andy Shevchenko wrote: > >> > The casting of __off64_t to unsigned long potentially wrong for values higher > >> > than ULONG_MAX. Let's fix that. > >> > >> i dont think this is the way to go. on 64bit systems, long long is > >> 128bits. i imagine the way to go (assuming we're always using LFS) is > >> to use PRIu64 from inttypes.h > > > > sizeof(unsigned long long) is 8 (64 bits) on my x86_64 fedora. > > so it is. i still think PRIu64 is the correct way to handle this as > there is no sizeof() assumption and no need for casting. Never used this, but yes, as long as this is something which has worked for ages and we are not going to have "this is not supported" issues - sure! But unsigned long long is 64 bits I think in all GNU systems, and casting to unsigned long long is quite standard practice AFAIK, so I do not see why it would be very bad thing to do.
On Mon, Apr 18, 2011 at 10:07, Artem Bityutskiy wrote: > On Mon, 2011-04-18 at 10:04 -0400, Mike Frysinger wrote: >> On Mon, Apr 18, 2011 at 09:55, Artem Bityutskiy wrote: >> > On Mon, 2011-04-18 at 09:49 -0400, Mike Frysinger wrote: >> >> On Mon, Apr 18, 2011 at 04:31, Andy Shevchenko wrote: >> >> > The casting of __off64_t to unsigned long potentially wrong for values higher >> >> > than ULONG_MAX. Let's fix that. >> >> >> >> i dont think this is the way to go. on 64bit systems, long long is >> >> 128bits. i imagine the way to go (assuming we're always using LFS) is >> >> to use PRIu64 from inttypes.h >> > >> > sizeof(unsigned long long) is 8 (64 bits) on my x86_64 fedora. >> >> so it is. i still think PRIu64 is the correct way to handle this as >> there is no sizeof() assumption and no need for casting. > > Never used this, but yes, as long as this is something which has worked > for ages and we are not going to have "this is not supported" issues - > sure! glibc has been shipping this since 1999 (if not earlier), so i dont think that'll be an issue. glibc-2.2.5 was released in 2002, and it's hard to even find that anymore. > But unsigned long long is 64 bits I think in all GNU systems, and > casting to unsigned long long is quite standard practice AFAIK, so I do > not see why it would be very bad thing to do. i think we should strive for the correct types in the printf string rather than attempting to cast things away all the time. casting things away can make us miss stuff, such as printf'ing a 64bit as a 32bit ;). -mike
On Mon, 2011-04-18 at 17:07 +0300, ext Artem Bityutskiy wrote: > On Mon, 2011-04-18 at 10:04 -0400, Mike Frysinger wrote: > > On Mon, Apr 18, 2011 at 09:55, Artem Bityutskiy wrote: > > > On Mon, 2011-04-18 at 09:49 -0400, Mike Frysinger wrote: > > >> On Mon, Apr 18, 2011 at 04:31, Andy Shevchenko wrote: > > >> > The casting of __off64_t to unsigned long potentially wrong for values higher > > >> > than ULONG_MAX. Let's fix that. > > >> > > >> i dont think this is the way to go. on 64bit systems, long long is > > >> 128bits. i imagine the way to go (assuming we're always using LFS) is > > >> to use PRIu64 from inttypes.h > > > > > > sizeof(unsigned long long) is 8 (64 bits) on my x86_64 fedora. > > > > so it is. i still think PRIu64 is the correct way to handle this as > > there is no sizeof() assumption and no need for casting. > > Never used this, but yes, as long as this is something which has worked > for ages and we are not going to have "this is not supported" issues - > sure! This is described in C99 ISO standard [1]. > But unsigned long long is 64 bits I think in all GNU systems, and > casting to unsigned long long is quite standard practice AFAIK, so I do > not see why it would be very bad thing to do. Yeah, some projects use following approach #ifndef PRIu64 #define PRIu64 "llu" #endif The opposite recommendation is to put "%9jd" classifier to print st_size field of struct stat [2]. [1] http://pubs.opengroup.org/onlinepubs/000095399/basedefs/inttypes.h.html [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html
On Mon, 2011-04-18 at 10:21 -0400, Mike Frysinger wrote: > On Mon, Apr 18, 2011 at 10:07, Artem Bityutskiy wrote: > > On Mon, 2011-04-18 at 10:04 -0400, Mike Frysinger wrote: > >> On Mon, Apr 18, 2011 at 09:55, Artem Bityutskiy wrote: > >> > On Mon, 2011-04-18 at 09:49 -0400, Mike Frysinger wrote: > >> >> On Mon, Apr 18, 2011 at 04:31, Andy Shevchenko wrote: > >> >> > The casting of __off64_t to unsigned long potentially wrong for values higher > >> >> > than ULONG_MAX. Let's fix that. > >> >> > >> >> i dont think this is the way to go. on 64bit systems, long long is > >> >> 128bits. i imagine the way to go (assuming we're always using LFS) is > >> >> to use PRIu64 from inttypes.h > >> > > >> > sizeof(unsigned long long) is 8 (64 bits) on my x86_64 fedora. > >> > >> so it is. i still think PRIu64 is the correct way to handle this as > >> there is no sizeof() assumption and no need for casting. > > > > Never used this, but yes, as long as this is something which has worked > > for ages and we are not going to have "this is not supported" issues - > > sure! > > glibc has been shipping this since 1999 (if not earlier), so i dont > think that'll be an issue. glibc-2.2.5 was released in 2002, and it's > hard to even find that anymore. > OK, sure, thanks for pointing this!
On Mon, Apr 18, 2011 at 10:26, Andy Shevchenko wrote: > [1] > http://pubs.opengroup.org/onlinepubs/000095399/basedefs/inttypes.h.html > [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html on the off chance you missed it, Issue 7 is out: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/inttypes.h.html -mike
On Mon, 2011-04-18 at 10:29 -0400, ext Mike Frysinger wrote: > On Mon, Apr 18, 2011 at 10:26, Andy Shevchenko wrote: > > [1] > > http://pubs.opengroup.org/onlinepubs/000095399/basedefs/inttypes.h.html > > [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html > > on the off chance you missed it, Issue 7 is out: > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/inttypes.h.html > -mike It's true. However it doesn't matter to our talk. The question is which way is better PRIu64 or %jd.
On Mon, Apr 18, 2011 at 10:50, Andy Shevchenko wrote: > On Mon, 2011-04-18 at 10:29 -0400, ext Mike Frysinger wrote: >> On Mon, Apr 18, 2011 at 10:26, Andy Shevchenko wrote: >> > [1] >> > http://pubs.opengroup.org/onlinepubs/000095399/basedefs/inttypes.h.html >> > [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html >> >> on the off chance you missed it, Issue 7 is out: >> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/inttypes.h.html > > It's true. However it doesn't matter to our talk. it doesnt ... i was just letting you know in case you werent aware. Issue 7 has a lot of fun new features. > The question is which way is better PRIu64 or %jd. %jd means [u]intmax_t which is not what this is ... on a 32bit system, int is 32bit. we're dealing with an explicitly 64bit type (off64_t), so let's go with an explicitly 64bit printf modifier. -mike
On Mon, 2011-04-18 at 10:57 -0400, ext Mike Frysinger wrote: > On Mon, Apr 18, 2011 at 10:50, Andy Shevchenko wrote: > > On Mon, 2011-04-18 at 10:29 -0400, ext Mike Frysinger wrote: > >> On Mon, Apr 18, 2011 at 10:26, Andy Shevchenko wrote: > >> > [1] > >> > http://pubs.opengroup.org/onlinepubs/000095399/basedefs/inttypes.h.html > >> > [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/printf.html > >> > >> on the off chance you missed it, Issue 7 is out: > >> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/inttypes.h.html > > > > It's true. However it doesn't matter to our talk. > > it doesnt ... i was just letting you know in case you werent aware. > Issue 7 has a lot of fun new features. > > > The question is which way is better PRIu64 or %jd. > > %jd means [u]intmax_t which is not what this is ... on a 32bit system, > int is 32bit. we're dealing with an explicitly 64bit type (off64_t), Accordingly to [1] Limits of greatest-width integer types * Minimum value of greatest-width signed integer type: {INTMAX_MIN} -(2 63 -1) * Maximum value of greatest-width signed integer type: {INTMAX_MAX} 2 63 -1 * Maximum value of greatest-width unsigned integer type: {UINTMAX_MAX} 2 64 -1 So, it fits to 64bit types. > so let's go with an explicitly 64bit printf modifier. Regarding to above I couldn't see any objection against %jd. At least one for PRIu64 - it looks awful in code. [1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html
On Mon, Apr 18, 2011 at 11:20, Andy Shevchenko wrote: > On Mon, 2011-04-18 at 10:57 -0400, ext Mike Frysinger wrote: >> so let's go with an explicitly 64bit printf modifier. > > Regarding to above I couldn't see any objection against %jd. At least > one for PRIu64 - it looks awful in code. it doesnt make sense to me to pair a non-explicit sized format string with an explicit sized type. off64_t is exactly 64bits, and PRIu64 prints exactly 64bits. that said, %ju is at least an improvement over the current code as well as the patch you've posted (doesnt seem to need casts on 32 or 64 bit systems). so if Artem is OK with that, i wont fight it. even though it's still not as correct as my proposal :P. -mike
On Mon, 2011-04-18 at 17:33 -0400, Mike Frysinger wrote: > On Mon, Apr 18, 2011 at 11:20, Andy Shevchenko wrote: > > On Mon, 2011-04-18 at 10:57 -0400, ext Mike Frysinger wrote: > >> so let's go with an explicitly 64bit printf modifier. > > > > Regarding to above I couldn't see any objection against %jd. At least > > one for PRIu64 - it looks awful in code. > > it doesnt make sense to me to pair a non-explicit sized format string > with an explicit sized type. off64_t is exactly 64bits, and PRIu64 > prints exactly 64bits. Yes, I agree. > that said, %ju is at least an improvement over the current code as > well as the patch you've posted (doesnt seem to need casts on 32 or 64 > bit systems). so if Artem is OK with that, i wont fight it. even > though it's still not as correct as my proposal :P. To be frank, I do not care much :-) We are talking about a single place. If Andy make a fix for all the mtd code, that would matter much much more :-) But yes, of course it is difficult to disagree that for explicitly sized type it is saner to use explicit-size print placeholder.
diff --git a/mkfs.jffs2.c b/mkfs.jffs2.c index 7bb9ad1..01db4be 100644 --- a/mkfs.jffs2.c +++ b/mkfs.jffs2.c @@ -1232,8 +1232,8 @@ static void recursive_populate_directory(struct filesystem_entry *dir) } else switch (e->sb.st_mode & S_IFMT) { case S_IFDIR: if (verbose) { - printf("\td %04o %9lu %5d:%-3d %s\n", - e->sb.st_mode & ~S_IFMT, (unsigned long) e->sb.st_size, + printf("\td %04o %llu %5d:%-3d %s\n", + e->sb.st_mode & ~S_IFMT, (unsigned long long) e->sb.st_size, (int) (e->sb.st_uid), (int) (e->sb.st_gid), e->name); } @@ -1242,8 +1242,8 @@ static void recursive_populate_directory(struct filesystem_entry *dir) break; case S_IFSOCK: if (verbose) { - printf("\ts %04o %9lu %5d:%-3d %s\n", - e->sb.st_mode & ~S_IFMT, (unsigned long) e->sb.st_size, + printf("\ts %04o %llu %5d:%-3d %s\n", + e->sb.st_mode & ~S_IFMT, (unsigned long long) e->sb.st_size, (int) e->sb.st_uid, (int) e->sb.st_gid, e->name); } write_pipe(e); @@ -1251,8 +1251,8 @@ static void recursive_populate_directory(struct filesystem_entry *dir) break; case S_IFIFO: if (verbose) { - printf("\tp %04o %9lu %5d:%-3d %s\n", - e->sb.st_mode & ~S_IFMT, (unsigned long) e->sb.st_size, + printf("\tp %04o %llu %5d:%-3d %s\n", + e->sb.st_mode & ~S_IFMT, (unsigned long long) e->sb.st_size, (int) e->sb.st_uid, (int) e->sb.st_gid, e->name); } write_pipe(e); @@ -1280,8 +1280,8 @@ static void recursive_populate_directory(struct filesystem_entry *dir) break; case S_IFLNK: if (verbose) { - printf("\tl %04o %9lu %5d:%-3d %s -> %s\n", - e->sb.st_mode & ~S_IFMT, (unsigned long) e->sb.st_size, + printf("\tl %04o %llu %5d:%-3d %s -> %s\n", + e->sb.st_mode & ~S_IFMT, (unsigned long long) e->sb.st_size, (int) e->sb.st_uid, (int) e->sb.st_gid, e->name, e->link); } @@ -1292,8 +1292,8 @@ static void recursive_populate_directory(struct filesystem_entry *dir) wrote = write_regular_file(e); write_xattr_entry(e); if (verbose) { - printf("\tf %04o %9lu (%9u) %5d:%-3d %s\n", - e->sb.st_mode & ~S_IFMT, (unsigned long) e->sb.st_size, + printf("\tf %04o %llu (%9u) %5d:%-3d %s\n", + e->sb.st_mode & ~S_IFMT, (unsigned long long) e->sb.st_size, wrote, (int) e->sb.st_uid, (int) e->sb.st_gid, e->name); } break;
The casting of __off64_t to unsigned long potentially wrong for values higher than ULONG_MAX. Let's fix that. Signed-off-by: Andy Shevchenko <ext-andriy.shevchenko@nokia.com> --- mkfs.jffs2.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)