Patchwork [PATCHv2,2/2] mkfs.jffs2: fix casting of __off64_t

login
register
mail settings
Submitter Shevchenko Andriy (EXT-Teleca/Helsinki)
Date April 18, 2011, 8:31 a.m.
Message ID <2285954c62e0401291aa3f5055bc79c6789149d7.1303115468.git.ext-andriy.shevchenko@nokia.com>
Download mbox | patch
Permalink /patch/91700/
State New
Headers show

Comments

Shevchenko Andriy (EXT-Teleca/Helsinki) - April 18, 2011, 8:31 a.m.
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(-)
Mike Frysinger - April 18, 2011, 1:49 p.m.
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
Artem Bityutskiy - April 18, 2011, 1:55 p.m.
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.
Mike Frysinger - April 18, 2011, 2:04 p.m.
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
Artem Bityutskiy - April 18, 2011, 2:07 p.m.
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.
Mike Frysinger - April 18, 2011, 2:21 p.m.
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
Shevchenko Andriy (EXT-Teleca/Helsinki) - April 18, 2011, 2:26 p.m.
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
Artem Bityutskiy - April 18, 2011, 2:29 p.m.
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!
Mike Frysinger - April 18, 2011, 2:29 p.m.
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
Shevchenko Andriy (EXT-Teleca/Helsinki) - April 18, 2011, 2:50 p.m.
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.
Mike Frysinger - April 18, 2011, 2:57 p.m.
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
Shevchenko Andriy (EXT-Teleca/Helsinki) - April 18, 2011, 3:20 p.m.
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
Mike Frysinger - April 18, 2011, 9:33 p.m.
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
Artem Bityutskiy - April 19, 2011, 6:43 a.m.
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.

Patch

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;