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

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

[blaisorblade_spam]

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 <blaisorblade_spam@yahoo.it>
---

 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
 
 /*
_

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

[Andrew Morton]

(cc'ed Neil)

blaisorblade_spam@yahoo.it 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Anton Altaparmakov]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3017 bytes --]

On Fri, 8 Oct 2004, Andrew Morton wrote:
> blaisorblade_spam@yahoo.it 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/

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

[Paolo Giarrusso]

On Friday 08 October 2004 22:11, Anton Altaparmakov wrote:
> On Fri, 8 Oct 2004, Andrew Morton wrote:
> > blaisorblade_spam@yahoo.it 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

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

[Anton Altaparmakov]

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:
> > > blaisorblade_spam@yahoo.it 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/

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

[BlaisorBlade]

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:
> > > > blaisorblade_spam@yahoo.it 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

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

[Anton Altaparmakov]

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:
> > > > > blaisorblade_spam@yahoo.it 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/

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

[Alasdair G Kergon]

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
-- 
agk@redhat.com

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

[Andrew Morton]

Alasdair G Kergon <agk@redhat.com> 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;

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

[Paolo Giarrusso]

On Wednesday 20 October 2004 00:52, Andrew Morton wrote:
> Alasdair G Kergon <agk@redhat.com> 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