2004-10-08 14:41:08

by Blaisorblade

[permalink] [raw]
Subject: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t


The Device Manager code barfs when sector_t is 64bit wide (i.e. an u64);
this can happen with CONFIG_LBD on i386, too, but sector_t is usually a long.
And region_t, chunk_t are typedefs for sector_t.

The problem is this code in drivers/md/dm.h (wouldn't we better fix the
FIXME?):

/*
* FIXME: I think this should be with the definition of sector_t
* in types.h.
*/
#ifdef CONFIG_LBD
#define SECTOR_FORMAT "%Lu"
#else
#define SECTOR_FORMAT "%lu"
#endif

Btw, x86_64 does not need to define sector_t on its own.
If there is any _good_ reason for that, the fix becomes adding a
#define SECTOR_FORMAT "%Lu"
in fact, gcc tries to be smart for warnings to ensure portability; so, even
when sizeof(long) == sizeof(long long), "%ld" and "%Ld" are different, for the
warnings).

Sample warnings (from both 2.6.8.1 and 2.6.9-rc2):
drivers/md/dm-raid1.c: In function `get_mirror':
drivers/md/dm-raid1.c:930: warning: long unsigned int format, sector_t arg (arg 3)
drivers/md/dm-raid1.c: In function `mirror_status':
drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 4)
drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 5)
drivers/md/dm-raid1.c:1206: warning: long unsigned int format, sector_t arg (arg 5)
drivers/md/dm-raid1.c:1212: warning: long unsigned int format, sector_t arg (arg 5)

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <[email protected]>
---

linux-2.6.9-current-paolo/drivers/md/dm.h | 10 ----------
linux-2.6.9-current-paolo/include/asm-h8300/types.h | 1 +
linux-2.6.9-current-paolo/include/asm-i386/types.h | 1 +
linux-2.6.9-current-paolo/include/asm-mips/types.h | 1 +
linux-2.6.9-current-paolo/include/asm-ppc/types.h | 1 +
linux-2.6.9-current-paolo/include/asm-s390/types.h | 1 +
linux-2.6.9-current-paolo/include/asm-sh/types.h | 1 +
linux-2.6.9-current-paolo/include/asm-x86_64/types.h | 3 ---
linux-2.6.9-current-paolo/include/linux/types.h | 1 +
9 files changed, 7 insertions(+), 13 deletions(-)

diff -puN drivers/md/dm.h~fix-dm-warnings drivers/md/dm.h
--- linux-2.6.9-current/drivers/md/dm.h~fix-dm-warnings 2004-10-08 16:39:05.888125592 +0200
+++ linux-2.6.9-current-paolo/drivers/md/dm.h 2004-10-08 16:39:05.942117384 +0200
@@ -22,16 +22,6 @@
#define DMEMIT(x...) sz += ((sz >= maxlen) ? \
0 : scnprintf(result + sz, maxlen - sz, x))

-/*
- * FIXME: I think this should be with the definition of sector_t
- * in types.h.
- */
-#ifdef CONFIG_LBD
-#define SECTOR_FORMAT "%Lu"
-#else
-#define SECTOR_FORMAT "%lu"
-#endif
-
#define SECTOR_SHIFT 9

/*
diff -puN include/asm-h8300/types.h~fix-dm-warnings include/asm-h8300/types.h
--- linux-2.6.9-current/include/asm-h8300/types.h~fix-dm-warnings 2004-10-08 16:39:05.902123464 +0200
+++ linux-2.6.9-current-paolo/include/asm-h8300/types.h 2004-10-08 16:39:05.959114800 +0200
@@ -56,6 +56,7 @@ typedef unsigned long long u64;
typedef u32 dma_addr_t;

#define HAVE_SECTOR_T
+#define SECTOR_FORMAT "%Lu"
typedef u64 sector_t;

typedef unsigned int kmem_bufctl_t;
diff -puN include/asm-i386/types.h~fix-dm-warnings include/asm-i386/types.h
--- linux-2.6.9-current/include/asm-i386/types.h~fix-dm-warnings 2004-10-08 16:39:05.914121640 +0200
+++ linux-2.6.9-current-paolo/include/asm-i386/types.h 2004-10-08 16:39:05.959114800 +0200
@@ -60,6 +60,7 @@ typedef u64 dma64_addr_t;

#ifdef CONFIG_LBD
typedef u64 sector_t;
+#define SECTOR_FORMAT "%Lu"
#define HAVE_SECTOR_T
#endif

diff -puN include/asm-mips/types.h~fix-dm-warnings include/asm-mips/types.h
--- linux-2.6.9-current/include/asm-mips/types.h~fix-dm-warnings 2004-10-08 16:39:05.915121488 +0200
+++ linux-2.6.9-current-paolo/include/asm-mips/types.h 2004-10-08 16:39:05.980111608 +0200
@@ -96,6 +96,7 @@ typedef unsigned long phys_t;

#ifdef CONFIG_LBD
typedef u64 sector_t;
+#define SECTOR_FORMAT "%Lu"
#define HAVE_SECTOR_T
#endif

diff -puN include/asm-ppc/types.h~fix-dm-warnings include/asm-ppc/types.h
--- linux-2.6.9-current/include/asm-ppc/types.h~fix-dm-warnings 2004-10-08 16:39:05.916121336 +0200
+++ linux-2.6.9-current-paolo/include/asm-ppc/types.h 2004-10-08 16:39:05.980111608 +0200
@@ -59,6 +59,7 @@ typedef u64 dma64_addr_t;

#ifdef CONFIG_LBD
typedef u64 sector_t;
+#define SECTOR_FORMAT "%Lu"
#define HAVE_SECTOR_T
#endif

diff -puN include/asm-s390/types.h~fix-dm-warnings include/asm-s390/types.h
--- linux-2.6.9-current/include/asm-s390/types.h~fix-dm-warnings 2004-10-08 16:39:05.917121184 +0200
+++ linux-2.6.9-current-paolo/include/asm-s390/types.h 2004-10-08 16:39:05.981111456 +0200
@@ -92,6 +92,7 @@ typedef union {

#ifdef CONFIG_LBD
typedef u64 sector_t;
+#define SECTOR_FORMAT "%Lu"
#define HAVE_SECTOR_T
#endif

diff -puN include/asm-sh/types.h~fix-dm-warnings include/asm-sh/types.h
--- linux-2.6.9-current/include/asm-sh/types.h~fix-dm-warnings 2004-10-08 16:39:05.920120728 +0200
+++ linux-2.6.9-current-paolo/include/asm-sh/types.h 2004-10-08 16:39:05.981111456 +0200
@@ -55,6 +55,7 @@ typedef u32 dma_addr_t;

#ifdef CONFIG_LBD
typedef u64 sector_t;
+#define SECTOR_FORMAT "%Lu"
#define HAVE_SECTOR_T
#endif

diff -puN include/asm-x86_64/types.h~fix-dm-warnings include/asm-x86_64/types.h
--- linux-2.6.9-current/include/asm-x86_64/types.h~fix-dm-warnings 2004-10-08 16:39:05.921120576 +0200
+++ linux-2.6.9-current-paolo/include/asm-x86_64/types.h 2004-10-08 16:39:05.981111456 +0200
@@ -48,9 +48,6 @@ typedef unsigned long long u64;
typedef u64 dma64_addr_t;
typedef u64 dma_addr_t;

-typedef u64 sector_t;
-#define HAVE_SECTOR_T
-
typedef unsigned short kmem_bufctl_t;

#endif /* __ASSEMBLY__ */
diff -puN include/linux/types.h~fix-dm-warnings include/linux/types.h
--- linux-2.6.9-current/include/linux/types.h~fix-dm-warnings 2004-10-08 16:39:05.939117840 +0200
+++ linux-2.6.9-current-paolo/include/linux/types.h 2004-10-08 16:39:05.959114800 +0200
@@ -130,6 +130,7 @@ typedef __s64 int64_t;
*/
#ifndef HAVE_SECTOR_T
typedef unsigned long sector_t;
+#define SECTOR_FORMAT "%lu"
#endif

/*
_


2004-10-08 19:21:21

by Andrew Morton

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t


(cc'ed Neil)

[email protected] wrote:
>
>
> The Device Manager code barfs when sector_t is 64bit wide (i.e. an u64);
> this can happen with CONFIG_LBD on i386, too, but sector_t is usually a long.
> And region_t, chunk_t are typedefs for sector_t.
>
> The problem is this code in drivers/md/dm.h (wouldn't we better fix the
> FIXME?):
>
> /*
> * FIXME: I think this should be with the definition of sector_t
> * in types.h.
> */
> #ifdef CONFIG_LBD
> #define SECTOR_FORMAT "%Lu"
> #else
> #define SECTOR_FORMAT "%lu"
> #endif
>
> Btw, x86_64 does not need to define sector_t on its own.
> If there is any _good_ reason for that, the fix becomes adding a
> #define SECTOR_FORMAT "%Lu"
> in fact, gcc tries to be smart for warnings to ensure portability; so, even
> when sizeof(long) == sizeof(long long), "%ld" and "%Ld" are different, for the
> warnings).
>
> Sample warnings (from both 2.6.8.1 and 2.6.9-rc2):
> drivers/md/dm-raid1.c: In function `get_mirror':
> drivers/md/dm-raid1.c:930: warning: long unsigned int format, sector_t arg (arg 3)
> drivers/md/dm-raid1.c: In function `mirror_status':
> drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 4)
> drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 5)
> drivers/md/dm-raid1.c:1206: warning: long unsigned int format, sector_t arg (arg 5)
> drivers/md/dm-raid1.c:1212: warning: long unsigned int format, sector_t arg (arg 5)
>

I would prefer that SECTOR_FORMAT be removed altogether.

The industry-standard way of printing a sector_t is:

printk("%llu", (unsigned long long)sector);

Or %Ld or %llo or whatever. The key point is that the format string should
specify long long and the argument should be typecast to long long.

The sector_t should not be typecast to u64, btw. We must never typecast
printk arguments to u64 because u64 is long long on some architectures and
long on others.


> diff -puN drivers/md/dm.h~fix-dm-warnings drivers/md/dm.h
> --- linux-2.6.9-current/drivers/md/dm.h~fix-dm-warnings 2004-10-08 16:39:05.888125592 +0200
> +++ linux-2.6.9-current-paolo/drivers/md/dm.h 2004-10-08 16:39:05.942117384 +0200
> @@ -22,16 +22,6 @@
> #define DMEMIT(x...) sz += ((sz >= maxlen) ? \
> 0 : scnprintf(result + sz, maxlen - sz, x))
>
> -/*
> - * FIXME: I think this should be with the definition of sector_t
> - * in types.h.
> - */
> -#ifdef CONFIG_LBD
> -#define SECTOR_FORMAT "%Lu"
> -#else
> -#define SECTOR_FORMAT "%lu"
> -#endif
> -
> #define SECTOR_SHIFT 9
>
> /*
> diff -puN include/asm-h8300/types.h~fix-dm-warnings include/asm-h8300/types.h
> --- linux-2.6.9-current/include/asm-h8300/types.h~fix-dm-warnings 2004-10-08 16:39:05.902123464 +0200
> +++ linux-2.6.9-current-paolo/include/asm-h8300/types.h 2004-10-08 16:39:05.959114800 +0200
> @@ -56,6 +56,7 @@ typedef unsigned long long u64;
> typedef u32 dma_addr_t;
>
> #define HAVE_SECTOR_T
> +#define SECTOR_FORMAT "%Lu"
> typedef u64 sector_t;
>
> typedef unsigned int kmem_bufctl_t;
> diff -puN include/asm-i386/types.h~fix-dm-warnings include/asm-i386/types.h
> --- linux-2.6.9-current/include/asm-i386/types.h~fix-dm-warnings 2004-10-08 16:39:05.914121640 +0200
> +++ linux-2.6.9-current-paolo/include/asm-i386/types.h 2004-10-08 16:39:05.959114800 +0200
> @@ -60,6 +60,7 @@ typedef u64 dma64_addr_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-mips/types.h~fix-dm-warnings include/asm-mips/types.h
> --- linux-2.6.9-current/include/asm-mips/types.h~fix-dm-warnings 2004-10-08 16:39:05.915121488 +0200
> +++ linux-2.6.9-current-paolo/include/asm-mips/types.h 2004-10-08 16:39:05.980111608 +0200
> @@ -96,6 +96,7 @@ typedef unsigned long phys_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-ppc/types.h~fix-dm-warnings include/asm-ppc/types.h
> --- linux-2.6.9-current/include/asm-ppc/types.h~fix-dm-warnings 2004-10-08 16:39:05.916121336 +0200
> +++ linux-2.6.9-current-paolo/include/asm-ppc/types.h 2004-10-08 16:39:05.980111608 +0200
> @@ -59,6 +59,7 @@ typedef u64 dma64_addr_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-s390/types.h~fix-dm-warnings include/asm-s390/types.h
> --- linux-2.6.9-current/include/asm-s390/types.h~fix-dm-warnings 2004-10-08 16:39:05.917121184 +0200
> +++ linux-2.6.9-current-paolo/include/asm-s390/types.h 2004-10-08 16:39:05.981111456 +0200
> @@ -92,6 +92,7 @@ typedef union {
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-sh/types.h~fix-dm-warnings include/asm-sh/types.h
> --- linux-2.6.9-current/include/asm-sh/types.h~fix-dm-warnings 2004-10-08 16:39:05.920120728 +0200
> +++ linux-2.6.9-current-paolo/include/asm-sh/types.h 2004-10-08 16:39:05.981111456 +0200
> @@ -55,6 +55,7 @@ typedef u32 dma_addr_t;
>
> #ifdef CONFIG_LBD
> typedef u64 sector_t;
> +#define SECTOR_FORMAT "%Lu"
> #define HAVE_SECTOR_T
> #endif
>
> diff -puN include/asm-x86_64/types.h~fix-dm-warnings include/asm-x86_64/types.h
> --- linux-2.6.9-current/include/asm-x86_64/types.h~fix-dm-warnings 2004-10-08 16:39:05.921120576 +0200
> +++ linux-2.6.9-current-paolo/include/asm-x86_64/types.h 2004-10-08 16:39:05.981111456 +0200
> @@ -48,9 +48,6 @@ typedef unsigned long long u64;
> typedef u64 dma64_addr_t;
> typedef u64 dma_addr_t;
>
> -typedef u64 sector_t;
> -#define HAVE_SECTOR_T
> -
> typedef unsigned short kmem_bufctl_t;
>
> #endif /* __ASSEMBLY__ */
> diff -puN include/linux/types.h~fix-dm-warnings include/linux/types.h
> --- linux-2.6.9-current/include/linux/types.h~fix-dm-warnings 2004-10-08 16:39:05.939117840 +0200
> +++ linux-2.6.9-current-paolo/include/linux/types.h 2004-10-08 16:39:05.959114800 +0200
> @@ -130,6 +130,7 @@ typedef __s64 int64_t;
> */
> #ifndef HAVE_SECTOR_T
> typedef unsigned long sector_t;
> +#define SECTOR_FORMAT "%lu"
> #endif
>
> /*
> _
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2004-10-08 20:12:06

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

On Fri, 8 Oct 2004, Andrew Morton wrote:
> [email protected] wrote:
> > The Device Manager code barfs when sector_t is 64bit wide (i.e. an u64);
> > this can happen with CONFIG_LBD on i386, too, but sector_t is usually a long.
> > And region_t, chunk_t are typedefs for sector_t.
> >
> > The problem is this code in drivers/md/dm.h (wouldn't we better fix the
> > FIXME?):
> >
> > /*
> > * FIXME: I think this should be with the definition of sector_t
> > * in types.h.
> > */
> > #ifdef CONFIG_LBD
> > #define SECTOR_FORMAT "%Lu"
> > #else
> > #define SECTOR_FORMAT "%lu"
> > #endif
> >
> > Btw, x86_64 does not need to define sector_t on its own.
> > If there is any _good_ reason for that, the fix becomes adding a
> > #define SECTOR_FORMAT "%Lu"
> > in fact, gcc tries to be smart for warnings to ensure portability; so, even
> > when sizeof(long) == sizeof(long long), "%ld" and "%Ld" are different, for the
> > warnings).
> >
> > Sample warnings (from both 2.6.8.1 and 2.6.9-rc2):
> > drivers/md/dm-raid1.c: In function `get_mirror':
> > drivers/md/dm-raid1.c:930: warning: long unsigned int format, sector_t arg (arg 3)
> > drivers/md/dm-raid1.c: In function `mirror_status':
> > drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 4)
> > drivers/md/dm-raid1.c:1200: warning: long unsigned int format, region_t arg (arg 5)
> > drivers/md/dm-raid1.c:1206: warning: long unsigned int format, sector_t arg (arg 5)
> > drivers/md/dm-raid1.c:1212: warning: long unsigned int format, sector_t arg (arg 5)
> >
>
> I would prefer that SECTOR_FORMAT be removed altogether.
>
> The industry-standard way of printing a sector_t is:
>
> printk("%llu", (unsigned long long)sector);
>
> Or %Ld or %llo or whatever. The key point is that the format string should
> specify long long and the argument should be typecast to long long.

Actually %Ld is completely wrong. I know in the kernel it makes no
difference but people see it in the kernel and then go off an use it in
userspace and it generates junk output on at least some architectures.
This is because %L means "long double (floating point)" not "long long
integer" and when you stuff an integer into it it goes wrong (on some
architectures)... From the printf(3) man page:

ll (ell-ell). A following integer conversion corre?
sponds to a long long int or unsigned long long int
argument, or a following n conversion corresponds
to a pointer to a long long int argument.

L A following a, A, e, E, f, F, g, or G conversion
corresponds to a long double argument. (C99 allows
%LF, but SUSv2 does not.)

Best regards,

Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

2004-10-08 20:45:38

by Paolo Giarrusso

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

On Friday 08 October 2004 22:11, Anton Altaparmakov wrote:
> On Fri, 8 Oct 2004, Andrew Morton wrote:
> > [email protected] wrote:


> Actually %Ld is completely wrong. I know in the kernel it makes no
> difference but people see it in the kernel and then go off an use it in
> userspace and it generates junk output on at least some architectures.
Well, gcc does not complain, and the problem is not "kernel is special" or "on
some arch it's different". It's an alias for "ll" for both gcc and glibc; I
checked, in fact, the version below of info pages for glibc:

This is Edition 0.10, last updated 2001-07-06, of `The GNU C Library
Reference Manual', for Version 2.3.x of the GNU C Library.
(I guess the "last update" is botched).

> This is because %L means "long double (floating point)" not "long long
> integer" and when you stuff an integer into it it goes wrong (on some
> architectures)...
I think an all ones, or at least on i386.
> From the printf(3) man page:
Outdated.
--
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729

2004-10-08 21:57:37

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

On Fri, 8 Oct 2004, Paolo Giarrusso wrote:
> On Friday 08 October 2004 22:11, Anton Altaparmakov wrote:
> > On Fri, 8 Oct 2004, Andrew Morton wrote:
> > > [email protected] wrote:
> > Actually %Ld is completely wrong. I know in the kernel it makes no
> > difference but people see it in the kernel and then go off an use it in
> > userspace and it generates junk output on at least some architectures.
> Well, gcc does not complain, and the problem is not "kernel is special" or "on
> some arch it's different". It's an alias for "ll" for both gcc and glibc; I
> checked, in fact, the version below of info pages for glibc:

gcc is not the only compiler and glibc is not the only C library.

> This is Edition 0.10, last updated 2001-07-06, of `The GNU C Library
> Reference Manual', for Version 2.3.x of the GNU C Library.
> (I guess the "last update" is botched).
>
> > This is because %L means "long double (floating point)" not "long long
> > integer" and when you stuff an integer into it it goes wrong (on some
> > architectures)...
> I think an all ones, or at least on i386.

Yes I know in the kernel and on i386 it makes no difference, I said that
already. But on some systems it does make a difference. I have seen it
myself and I have had it reported. Thinking about it when I said
architectures I possibly meant to say "other Unix flavours", I think one
of the *BSDs was the one where I saw the difference between %L and %ll
manifest itself.

> > From the printf(3) man page:
> Outdated.

Sorry, it is not. I find it somewhat strange that you choose gcc and
glibc to say what is correct... Ever heard of standards?!?

Quoting from C99 standard (ISO/IEC 9899:1999(E)):

[cut here]
ll (ell-ell) Specifies that a following d, i, o, u, x, or X conversion
specifier applies to a long long int or unsigned long long
int argument; or that a following n conversion specifier
applies to a pointer to a long long int argument.
[snip]
L Specifies that a following a, A, e, E, f, F, g, or G
conversion specifier applies to a long double argument.

If a length modifier appears with any conversion specifier other than as
specified above, the behavior is undefined.
[cut here]

So the C99 standard specifies the use of %L with an integer type
conversion specified is undefined. So relying on %L being an alias for
%ll considering there are systems where this is not the case seems stupid
to me but hey I don't really care. I just thought I would let people who
don't know it know. If you want to carry on using %L because it works in
the kernel be my guest.

Best regards,

Anton

PS. Just don't submit patches containing %L for fs/ntfs/* or I will flame
you to crisp as we share code with userspace libntfs and ntfsprogs and I
and other NTFS developers care about our code being portable and working
on as many architectures and OS as possible. (-;

--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

2004-10-09 00:04:42

by Blaisorblade

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

On Friday 08 October 2004 23:57, Anton Altaparmakov wrote:
> On Fri, 8 Oct 2004, Paolo Giarrusso wrote:
> > On Friday 08 October 2004 22:11, Anton Altaparmakov wrote:
> > > On Fri, 8 Oct 2004, Andrew Morton wrote:
> > > > [email protected] wrote:

> Yes I know in the kernel and on i386 it makes no difference, I said that
> already. But on some systems it does make a difference. I have seen it
> myself and I have had it reported.

> Thinking about it when I said
> architectures I possibly meant to say "other Unix flavours", I think one
> of the *BSDs was the one where I saw the difference between %L and %ll
> manifest itself.
Ok, I thought hardware archs - for other Unixes you're right.

Sorry for this and thanks for the lesson. Bye
> Sorry, it is not. I find it somewhat strange that you choose gcc and
> glibc to say what is correct... Ever heard of standards?!?
Yes, I heard them, I just never bought ISO standards.

--
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729

2004-10-09 06:38:16

by Anton Altaparmakov

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

On Sat, 9 Oct 2004, BlaisorBlade wrote:
> On Friday 08 October 2004 23:57, Anton Altaparmakov wrote:
> > On Fri, 8 Oct 2004, Paolo Giarrusso wrote:
> > > On Friday 08 October 2004 22:11, Anton Altaparmakov wrote:
> > > > On Fri, 8 Oct 2004, Andrew Morton wrote:
> > > > > [email protected] wrote:
>
> > Yes I know in the kernel and on i386 it makes no difference, I said that
> > already. But on some systems it does make a difference. I have seen it
> > myself and I have had it reported.
>
> > Thinking about it when I said
> > architectures I possibly meant to say "other Unix flavours", I think one
> > of the *BSDs was the one where I saw the difference between %L and %ll
> > manifest itself.
> Ok, I thought hardware archs - for other Unixes you're right.
>
> Sorry for this and thanks for the lesson. Bye
> > Sorry, it is not. I find it somewhat strange that you choose gcc and
> > glibc to say what is correct... Ever heard of standards?!?
> Yes, I heard them, I just never bought ISO standards.

Just search google and you will find plenty of places offering them for
free download...

Best regards,

Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

2004-10-19 17:42:57

by Alasdair G Kergon

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

On Fri, Oct 08, 2004 at 12:12:39PM -0700, Andrew Morton wrote:
> I would prefer that SECTOR_FORMAT be removed altogether.

> The industry-standard way of printing a sector_t is:
> printk("%llu", (unsigned long long)sector);

What about reading a sector_t with sscanf, which looks like the
reason for the existence of SECTOR_FORMAT?

Alasdair
--
[email protected]

2004-10-20 00:02:24

by Andrew Morton

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

Alasdair G Kergon <[email protected]> wrote:
>
> On Fri, Oct 08, 2004 at 12:12:39PM -0700, Andrew Morton wrote:
> > I would prefer that SECTOR_FORMAT be removed altogether.
>
> > The industry-standard way of printing a sector_t is:
> > printk("%llu", (unsigned long long)sector);
>
> What about reading a sector_t with sscanf, which looks like the
> reason for the existence of SECTOR_FORMAT?

I'm not sure that it has arisen. I'd do:

unsigned long long temp;
sector_t sector;

sscanf(buf, "%llu", &temp);
sector = temp;

2004-10-21 15:35:24

by Paolo Giarrusso

[permalink] [raw]
Subject: Re: [patch 1/1] dm: fix printk warnings about whether %lu/%Lu is right for sector_t

On Wednesday 20 October 2004 00:52, Andrew Morton wrote:
> Alasdair G Kergon <[email protected]> wrote:
> > On Fri, Oct 08, 2004 at 12:12:39PM -0700, Andrew Morton wrote:
> > > I would prefer that SECTOR_FORMAT be removed altogether.
> > >
> > > The industry-standard way of printing a sector_t is:
> > > printk("%llu", (unsigned long long)sector);
> >
> > What about reading a sector_t with sscanf, which looks like the
> > reason for the existence of SECTOR_FORMAT?
>
> I'm not sure that it has arisen. I'd do:

> unsigned long long temp;
> sector_t sector;
>
> sscanf(buf, "%llu", &temp);
> sector = temp;

It is already meaningless, IMHO, in this short example. But when you have such
code:

if (sscanf(argv[2], SECTOR_FORMAT, &cc->iv_offset) != 1) {
ti->error = PFX "Invalid iv_offset sector";
goto bad4;
}

if (sscanf(argv[4], SECTOR_FORMAT, &cc->start) != 1) {
ti->error = PFX "Invalid device sector";
goto bad4;
}

it becomes even uglier.

Why cannot we simply define the macro in linux/types.h and go along? There is
no reason at all for arch-dependent definition of this macro; we can handle
CONFIG_LBD in arch-independent code. In fact, it's what I'm doing.

I'm resending the patch, removing altogether HAVE_SECTOR_T and anything in
arch-code referring to that. For h8300, I'm unconditionally defining
CONFIG_LBD in Kconfig.

An even better road would would be to use always a "unsigned long long" for
64-bit sector_t even on 64-bit archs.

Is there any reason for having u/s64 being "(unsigned) long" instead of
"(unsigned) long long"? I.e., must we cope with braindead gcc? If not, this
should be fixed, to avoid even other such problems.

Bye
--
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729