[patch] fat/msdos/vfat crud removal

[patch] fat/msdos/vfat crud removal

[Albert D. Cahalan]

This get rid of the old byte order macros.

diff -Naurd old/fs/fat/cache.c new/fs/fat/cache.c
--- old/fs/fat/cache.c	Sun Jun  2 21:44:45 2002
+++ new/fs/fat/cache.c	Sat Jun  8 17:25:48 2002
@@ -67,13 +67,13 @@
 	}
 	if (sbi->fat_bits == 32) {
 		p_first = p_last = NULL; /* GCC needs that stuff */
-		next = CF_LE_L(((__u32 *) bh->b_data)[(first &
+		next = le32_to_cpu(((__u32 *) bh->b_data)[(first &
 		    (sb->s_blocksize - 1)) >> 2]);
 		/* Fscking Microsoft marketing department. Their "32" is 28. */
 		next &= 0x0fffffff;
 	} else if (sbi->fat_bits == 16) {
 		p_first = p_last = NULL; /* GCC needs that stuff */
-		next = CF_LE_W(((__u16 *) bh->b_data)[(first &
+		next = le16_to_cpu(((__u16 *) bh->b_data)[(first &
 		    (sb->s_blocksize - 1)) >> 1]);
 	} else {
 		p_first = &((__u8 *)bh->b_data)[first & (sb->s_blocksize - 1)];
@@ -86,10 +86,10 @@
 	if (new_value != -1) {
 		if (sbi->fat_bits == 32) {
 			((__u32 *)bh->b_data)[(first & (sb->s_blocksize - 1)) >> 2]
-				= CT_LE_L(new_value);
+				= cpu_to_le32(new_value);
 		} else if (sbi->fat_bits == 16) {
 			((__u16 *)bh->b_data)[(first & (sb->s_blocksize - 1)) >> 1]
-				= CT_LE_W(new_value);
+				= cpu_to_le16(new_value);
 		} else {
 			if (nr & 1) {
 				*p_first = (*p_first & 0xf) | (new_value << 4);
diff -Naurd old/fs/fat/dir.c new/fs/fat/dir.c
--- old/fs/fat/dir.c	Sun Jun  2 21:44:40 2002
+++ new/fs/fat/dir.c	Sat Jun  8 17:25:48 2002
@@ -768,17 +768,17 @@
 	memcpy(de[0].name,MSDOS_DOT,MSDOS_NAME);
 	memcpy(de[1].name,MSDOS_DOTDOT,MSDOS_NAME);
 	de[0].attr = de[1].attr = ATTR_DIR;
-	de[0].time = de[1].time = CT_LE_W(time);
-	de[0].date = de[1].date = CT_LE_W(date);
+	de[0].time = de[1].time = cpu_to_le16(time);
+	de[0].date = de[1].date = cpu_to_le16(date);
 	if (is_vfat) {	/* extra timestamps */
-		de[0].ctime = de[1].ctime = CT_LE_W(time);
+		de[0].ctime = de[1].ctime = cpu_to_le16(time);
 		de[0].adate = de[0].cdate =
-			de[1].adate = de[1].cdate = CT_LE_W(date);
+			de[1].adate = de[1].cdate = cpu_to_le16(date);
 	}
-	de[0].start = CT_LE_W(MSDOS_I(dir)->i_logstart);
-	de[0].starthi = CT_LE_W(MSDOS_I(dir)->i_logstart>>16);
-	de[1].start = CT_LE_W(MSDOS_I(parent)->i_logstart);
-	de[1].starthi = CT_LE_W(MSDOS_I(parent)->i_logstart>>16);
+	de[0].start = cpu_to_le16(MSDOS_I(dir)->i_logstart);
+	de[0].starthi = cpu_to_le16(MSDOS_I(dir)->i_logstart>>16);
+	de[1].start = cpu_to_le16(MSDOS_I(parent)->i_logstart);
+	de[1].starthi = cpu_to_le16(MSDOS_I(parent)->i_logstart>>16);
 	fat_mark_buffer_dirty(sb, bh);
 	fat_brelse(sb, bh);
 	dir->i_atime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
diff -Naurd old/fs/fat/inode.c new/fs/fat/inode.c
--- old/fs/fat/inode.c	Sun Jun  2 21:44:48 2002
+++ new/fs/fat/inode.c	Sat Jun  8 17:25:48 2002
@@ -712,7 +712,7 @@
 		goto out_invalid;
 	}
 	logical_sector_size =
-		CF_LE_W(get_unaligned((unsigned short *) &b->sector_size));
+		le16_to_cpu(get_unaligned((unsigned short *) &b->sector_size));
 	if (!logical_sector_size
 	    || (logical_sector_size & (logical_sector_size - 1))
 	    || (logical_sector_size < 512)
@@ -760,8 +760,8 @@
 	sbi->cluster_bits = ffs(sb->s_blocksize * sbi->cluster_size) - 1;
 	sbi->fats = b->fats;
 	sbi->fat_bits = 0;		/* Don't know yet */
-	sbi->fat_start = CF_LE_W(b->reserved);
-	sbi->fat_length = CF_LE_W(b->fat_length);
+	sbi->fat_start = le16_to_cpu(b->reserved);
+	sbi->fat_length = le16_to_cpu(b->fat_length);
 	sbi->root_cluster = 0;
 	sbi->free_clusters = -1;	/* Don't know yet */
 	sbi->prev_free = 0;
@@ -772,11 +772,11 @@
 
 		/* Must be FAT32 */
 		sbi->fat_bits = 32;
-		sbi->fat_length = CF_LE_L(b->fat32_length);
-		sbi->root_cluster = CF_LE_L(b->root_cluster);
+		sbi->fat_length = le32_to_cpu(b->fat32_length);
+		sbi->root_cluster = le32_to_cpu(b->root_cluster);
 
 		/* MC - if info_sector is 0, don't multiply by 0 */
-		sbi->fsinfo_sector = CF_LE_W(b->info_sector);
+		sbi->fsinfo_sector = le16_to_cpu(b->info_sector);
 		if (sbi->fsinfo_sector == 0)
 			sbi->fsinfo_sector = 1;
 
@@ -793,11 +793,11 @@
 			printk("FAT: Did not find valid FSINFO signature.\n"
 			       "     Found signature1 0x%08x signature2 0x%08x"
 			       " (sector = %lu)\n",
-			       CF_LE_L(fsinfo->signature1),
-			       CF_LE_L(fsinfo->signature2),
+			       le32_to_cpu(fsinfo->signature1),
+			       le32_to_cpu(fsinfo->signature2),
 			       sbi->fsinfo_sector);
 		} else {
-			sbi->free_clusters = CF_LE_L(fsinfo->free_clusters);
+			sbi->free_clusters = le32_to_cpu(fsinfo->free_clusters);
 		}
 
 		brelse(fsinfo_bh);
@@ -808,7 +808,7 @@
 
 	sbi->dir_start = sbi->fat_start + sbi->fats * sbi->fat_length;
 	sbi->dir_entries =
-		CF_LE_W(get_unaligned((unsigned short *)&b->dir_entries));
+		le16_to_cpu(get_unaligned((unsigned short *)&b->dir_entries));
 	if (sbi->dir_entries & (sbi->dir_per_block - 1)) {
 		printk("FAT: bogus directroy-entries per block\n");
 		brelse(bh);
@@ -818,9 +818,9 @@
 	rootdir_sectors = sbi->dir_entries
 		* sizeof(struct msdos_dir_entry) / sb->s_blocksize;
 	sbi->data_start = sbi->dir_start + rootdir_sectors;
-	total_sectors = CF_LE_W(get_unaligned((unsigned short *)&b->sectors));
+	total_sectors = le16_to_cpu(get_unaligned((unsigned short *)&b->sectors));
 	if (total_sectors == 0)
-		total_sectors = CF_LE_L(b->total_sect);
+		total_sectors = le32_to_cpu(b->total_sect);
 	sbi->clusters = (total_sectors - sbi->data_start) / sbi->cluster_size;
 
 	if (sbi->fat_bits != 32)
@@ -1018,10 +1018,10 @@
 		inode->i_op = sbi->dir_ops;
 		inode->i_fop = &fat_dir_operations;
 
-		MSDOS_I(inode)->i_start = CF_LE_W(de->start);
+		MSDOS_I(inode)->i_start = le16_to_cpu(de->start);
 		if (sbi->fat_bits == 32) {
 			MSDOS_I(inode)->i_start |=
-				(CF_LE_W(de->starthi) << 16);
+				(le16_to_cpu(de->starthi) << 16);
 		}
 		MSDOS_I(inode)->i_logstart = MSDOS_I(inode)->i_start;
 		error = fat_calc_dir_size(inode);
@@ -1044,13 +1044,13 @@
 		       !is_exec(de->ext))
 		    	? S_IRUGO|S_IWUGO : S_IRWXUGO)
 		    & ~sbi->options.fs_umask) | S_IFREG;
-		MSDOS_I(inode)->i_start = CF_LE_W(de->start);
+		MSDOS_I(inode)->i_start = le16_to_cpu(de->start);
 		if (sbi->fat_bits == 32) {
 			MSDOS_I(inode)->i_start |=
-				(CF_LE_W(de->starthi) << 16);
+				(le16_to_cpu(de->starthi) << 16);
 		}
 		MSDOS_I(inode)->i_logstart = MSDOS_I(inode)->i_start;
-		inode->i_size = CF_LE_L(de->size);
+		inode->i_size = le32_to_cpu(de->size);
 	        inode->i_op = &fat_file_inode_operations;
 	        inode->i_fop = &fat_file_operations;
 		inode->i_mapping->a_ops = &fat_aops;
@@ -1065,10 +1065,10 @@
 	inode->i_blocks = ((inode->i_size + inode->i_blksize - 1)
 			   & ~(inode->i_blksize - 1)) >> 9;
 	inode->i_mtime = inode->i_atime =
-		date_dos2unix(CF_LE_W(de->time),CF_LE_W(de->date));
+		date_dos2unix(le16_to_cpu(de->time),le16_to_cpu(de->date));
 	inode->i_ctime =
 		MSDOS_SB(sb)->options.isvfat
-		? date_dos2unix(CF_LE_W(de->ctime),CF_LE_W(de->cdate))
+		? date_dos2unix(le16_to_cpu(de->ctime),le16_to_cpu(de->cdate))
 		: inode->i_mtime;
 	MSDOS_I(inode)->i_ctime_ms = de->ctime_ms;
 
@@ -1110,20 +1110,20 @@
 	}
 	else {
 		raw_entry->attr = ATTR_NONE;
-		raw_entry->size = CT_LE_L(inode->i_size);
+		raw_entry->size = cpu_to_le32(inode->i_size);
 	}
 	raw_entry->attr |= MSDOS_MKATTR(inode->i_mode) |
 	    MSDOS_I(inode)->i_attrs;
-	raw_entry->start = CT_LE_W(MSDOS_I(inode)->i_logstart);
-	raw_entry->starthi = CT_LE_W(MSDOS_I(inode)->i_logstart >> 16);
+	raw_entry->start = cpu_to_le16(MSDOS_I(inode)->i_logstart);
+	raw_entry->starthi = cpu_to_le16(MSDOS_I(inode)->i_logstart >> 16);
 	fat_date_unix2dos(inode->i_mtime,&raw_entry->time,&raw_entry->date);
-	raw_entry->time = CT_LE_W(raw_entry->time);
-	raw_entry->date = CT_LE_W(raw_entry->date);
+	raw_entry->time = cpu_to_le16(raw_entry->time);
+	raw_entry->date = cpu_to_le16(raw_entry->date);
 	if (MSDOS_SB(sb)->options.isvfat) {
 		fat_date_unix2dos(inode->i_ctime,&raw_entry->ctime,&raw_entry->cdate);
 		raw_entry->ctime_ms = MSDOS_I(inode)->i_ctime_ms;
-		raw_entry->ctime = CT_LE_W(raw_entry->ctime);
-		raw_entry->cdate = CT_LE_W(raw_entry->cdate);
+		raw_entry->ctime = cpu_to_le16(raw_entry->ctime);
+		raw_entry->cdate = cpu_to_le16(raw_entry->cdate);
 	}
 	spin_unlock(&fat_inode_lock);
 	fat_mark_buffer_dirty(sb, bh);
diff -Naurd old/fs/fat/misc.c new/fs/fat/misc.c
--- old/fs/fat/misc.c	Sun Jun  2 21:44:52 2002
+++ new/fs/fat/misc.c	Sat Jun  8 17:25:48 2002
@@ -115,10 +115,10 @@
 		printk("FAT: Did not find valid FSINFO signature.\n"
 		       "     Found signature1 0x%08x signature2 0x%08x"
 		       " (sector = %lu)\n",
-		       CF_LE_L(fsinfo->signature1), CF_LE_L(fsinfo->signature2),
+		       le32_to_cpu(fsinfo->signature1), le32_to_cpu(fsinfo->signature2),
 		       MSDOS_SB(sb)->fsinfo_sector);
 	} else {
-		fsinfo->free_clusters = CF_LE_L(MSDOS_SB(sb)->free_clusters);
+		fsinfo->free_clusters = le32_to_cpu(MSDOS_SB(sb)->free_clusters);
 		fat_mark_buffer_dirty(sb, bh);
 	}
 	fat_brelse(sb, bh);
@@ -405,9 +405,9 @@
     done = !IS_FREE(data[entry].name) \
       && ( \
            ( \
-             (MSDOS_SB(sb)->fat_bits != 32) ? 0 : (CF_LE_W(data[entry].starthi) << 16) \
+             (MSDOS_SB(sb)->fat_bits != 32) ? 0 : (le16_to_cpu(data[entry].starthi) << 16) \
            ) \
-           | CF_LE_W(data[entry].start) \
+           | le16_to_cpu(data[entry].start) \
          ) == *number;
 
 #define RSS_FREE /* search for free entry */ \
@@ -447,9 +447,9 @@
 		if (done) {
 			if (ino)
 				*ino = sector * MSDOS_SB(sb)->dir_per_block + entry;
-			start = CF_LE_W(data[entry].start);
+			start = le16_to_cpu(data[entry].start);
 			if (MSDOS_SB(sb)->fat_bits == 32) {
-				start |= (CF_LE_W(data[entry].starthi) << 16);
+				start |= (le16_to_cpu(data[entry].starthi) << 16);
 			}
 			if (!res_bh)
 				fat_brelse(sb, bh);
diff -Naurd old/fs/msdos/namei.c new/fs/msdos/namei.c
--- old/fs/msdos/namei.c	Sun Jun  2 21:44:49 2002
+++ new/fs/msdos/namei.c	Sat Jun  8 17:25:48 2002
@@ -514,8 +514,8 @@
 		mark_inode_dirty(new_inode);
 	}
 	if (dotdot_bh) {
-		dotdot_de->start = CT_LE_W(MSDOS_I(new_dir)->i_logstart);
-		dotdot_de->starthi = CT_LE_W((MSDOS_I(new_dir)->i_logstart) >> 16);
+		dotdot_de->start = cpu_to_le16(MSDOS_I(new_dir)->i_logstart);
+		dotdot_de->starthi = cpu_to_le16((MSDOS_I(new_dir)->i_logstart) >> 16);
 		fat_mark_buffer_dirty(sb, dotdot_bh);
 		old_dir->i_nlink--;
 		mark_inode_dirty(old_dir);
diff -Naurd old/fs/vfat/namei.c new/fs/vfat/namei.c
--- old/fs/vfat/namei.c	Sun Jun  2 21:44:51 2002
+++ new/fs/vfat/namei.c	Sat Jun  8 17:25:48 2002
@@ -1257,8 +1257,8 @@
 
 	if (is_dir) {
 		int start = MSDOS_I(new_dir)->i_logstart;
-		dotdot_de->start = CT_LE_W(start);
-		dotdot_de->starthi = CT_LE_W(start>>16);
+		dotdot_de->start = cpu_to_le16(start);
+		dotdot_de->starthi = cpu_to_le16(start>>16);
 		fat_mark_buffer_dirty(sb, dotdot_bh);
 		old_dir->i_nlink--;
 		if (new_inode) {
diff -Naurd old/include/linux/msdos_fs.h new/include/linux/msdos_fs.h
--- old/include/linux/msdos_fs.h	Sun Jun  2 21:44:52 2002
+++ new/include/linux/msdos_fs.h	Sat Jun  8 17:28:07 2002
@@ -80,8 +80,8 @@
 
 #define FAT_FSINFO_SIG1		0x41615252
 #define FAT_FSINFO_SIG2		0x61417272
-#define IS_FSINFO(x)	(CF_LE_L((x)->signature1) == FAT_FSINFO_SIG1	\
-			 && CF_LE_L((x)->signature2) == FAT_FSINFO_SIG2)
+#define IS_FSINFO(x)	(le32_to_cpu((x)->signature1) == FAT_FSINFO_SIG1	\
+			 && le32_to_cpu((x)->signature2) == FAT_FSINFO_SIG2)
 
 /*
  * ioctl commands
@@ -98,17 +98,6 @@
 #define VFAT_SFN_CREATE_WIN95	0x0100 /* emulate win95 rule for create */
 #define VFAT_SFN_CREATE_WINNT	0x0200 /* emulate winnt rule for create */
 
-/*
- * Conversion from and to little-endian byte order. (no-op on i386/i486)
- *
- * Naming: Ca_b_c, where a: F = from, T = to, b: LE = little-endian,
- * BE = big-endian, c: W = word (16 bits), L = longword (32 bits)
- */
-
-#define CF_LE_W(v) le16_to_cpu(v)
-#define CF_LE_L(v) le32_to_cpu(v)
-#define CT_LE_W(v) cpu_to_le16(v)
-#define CT_LE_L(v) cpu_to_le32(v)
 
 struct fat_boot_sector {
 	__s8	ignored[3];	/* Boot strap short or near jump */

Re: [patch] fat/msdos/vfat crud removal

[OGAWA Hirofumi]

"Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

> This get rid of the old byte order macros.
> 
> diff -Naurd old/fs/fat/cache.c new/fs/fat/cache.c
> --- old/fs/fat/cache.c	Sun Jun  2 21:44:45 2002
> +++ new/fs/fat/cache.c	Sat Jun  8 17:25:48 2002
> @@ -67,13 +67,13 @@
>  	}
>  	if (sbi->fat_bits == 32) {
>  		p_first = p_last = NULL; /* GCC needs that stuff */
> -		next = CF_LE_L(((__u32 *) bh->b_data)[(first &
> +		next = le32_to_cpu(((__u32 *) bh->b_data)[(first &
>  		    (sb->s_blocksize - 1)) >> 2]);
>  		/* Fscking Microsoft marketing department. Their "32" is 28. */

Personally I think this patch makes code readable. But please don't
remove BT
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] fat/msdos/vfat crud removal

[OGAWA Hirofumi]

Sorry, my previous miss email.

"Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

> This get rid of the old byte order macros.
> 
> diff -Naurd old/fs/fat/cache.c new/fs/fat/cache.c
> --- old/fs/fat/cache.c	Sun Jun  2 21:44:45 2002
> +++ new/fs/fat/cache.c	Sat Jun  8 17:25:48 2002
> @@ -67,13 +67,13 @@
>  	}
>  	if (sbi->fat_bits == 32) {
>  		p_first = p_last = NULL; /* GCC needs that stuff */
> -		next = CF_LE_L(((__u32 *) bh->b_data)[(first &
> +		next = le32_to_cpu(((__u32 *) bh->b_data)[(first &
>  		    (sb->s_blocksize - 1)) >> 2]);

[...]

> -/*
> - * Conversion from and to little-endian byte order. (no-op on i386/i486)
> - *
> - * Naming: Ca_b_c, where a: F = from, T = to, b: LE = little-endian,
> - * BE = big-endian, c: W = word (16 bits), L = longword (32 bits)
> - */
> -
> -#define CF_LE_W(v) le16_to_cpu(v)
> -#define CF_LE_L(v) le32_to_cpu(v)
> -#define CT_LE_W(v) cpu_to_le16(v)
> -#define CT_LE_L(v) cpu_to_le32(v)

Personally I think this patch makes code readable. But please don't
remove Cx_LE_x macros. Cx_LE_x is used from dosfsck.

The following incrementale patch fixes above problem, and does trivial
cleanup.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

diff -urN linux-2.5.20/fs/fat/inode.c fat-byte_order/fs/fat/inode.c
--- linux-2.5.20/fs/fat/inode.c	Sun Jun  9 13:05:33 2002
+++ fat-byte_order/fs/fat/inode.c	Sun Jun  9 13:19:16 2002
@@ -711,8 +711,7 @@
 		brelse(bh);
 		goto out_invalid;
 	}
-	logical_sector_size =
-		le16_to_cpu(get_unaligned((unsigned short *) &b->sector_size));
+	logical_sector_size = le16_to_cpu(get_unaligned((u16*)&b->sector_size));
 	if (!logical_sector_size
 	    || (logical_sector_size & (logical_sector_size - 1))
 	    || (logical_sector_size < 512)
@@ -807,8 +806,7 @@
 	sbi->dir_per_block_bits = ffs(sbi->dir_per_block) - 1;
 
 	sbi->dir_start = sbi->fat_start + sbi->fats * sbi->fat_length;
-	sbi->dir_entries =
-		le16_to_cpu(get_unaligned((unsigned short *)&b->dir_entries));
+	sbi->dir_entries = le16_to_cpu(get_unaligned((u16 *)&b->dir_entries));
 	if (sbi->dir_entries & (sbi->dir_per_block - 1)) {
 		printk("FAT: bogus directroy-entries per block\n");
 		brelse(bh);
@@ -818,7 +816,7 @@
 	rootdir_sectors = sbi->dir_entries
 		* sizeof(struct msdos_dir_entry) / sb->s_blocksize;
 	sbi->data_start = sbi->dir_start + rootdir_sectors;
-	total_sectors = le16_to_cpu(get_unaligned((unsigned short *)&b->sectors));
+	total_sectors = le16_to_cpu(get_unaligned((u16 *)&b->sectors));
 	if (total_sectors == 0)
 		total_sectors = le32_to_cpu(b->total_sect);
 	sbi->clusters = (total_sectors - sbi->data_start) / sbi->cluster_size;
diff -urN linux-2.5.20/include/linux/msdos_fs.h fat-byte_order/include/linux/msdos_fs.h
--- linux-2.5.20/include/linux/msdos_fs.h	Sun Jun  9 13:05:34 2002
+++ fat-byte_order/include/linux/msdos_fs.h	Sun Jun  9 13:02:13 2002
@@ -13,6 +13,11 @@
 #define MSDOS_DPB_BITS	4		/* log2(MSDOS_DPB) */
 #define MSDOS_DPS	(SECTOR_SIZE / sizeof(struct msdos_dir_entry))
 #define MSDOS_DPS_BITS	4		/* log2(MSDOS_DPS) */
+#define CF_LE_W(v) le16_to_cpu(v)
+#define CF_LE_L(v) le32_to_cpu(v)
+#define CT_LE_W(v) cpu_to_le16(v)
+#define CT_LE_L(v) cpu_to_le32(v)
+
 
 #define MSDOS_ROOT_INO	1	/* == MINIX_ROOT_INO */
 #define MSDOS_DIR_BITS	5	/* log2(sizeof(struct msdos_dir_entry)) */
@@ -80,7 +85,7 @@
 
 #define FAT_FSINFO_SIG1		0x41615252
 #define FAT_FSINFO_SIG2		0x61417272
-#define IS_FSINFO(x)	(le32_to_cpu((x)->signature1) == FAT_FSINFO_SIG1	\
+#define IS_FSINFO(x)	(le32_to_cpu((x)->signature1) == FAT_FSINFO_SIG1 \
 			 && le32_to_cpu((x)->signature2) == FAT_FSINFO_SIG2)
 
 /*

Re: [patch] fat/msdos/vfat crud removal

[Albert D. Cahalan]

OGAWA Hirofumi writes:
> "Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

>> - * Conversion from and to little-endian byte order. (no-op on i386/i486)
>> - *
>> - * Naming: Ca_b_c, where a: F = from, T = to, b: LE = little-endian,
>> - * BE = big-endian, c: W = word (16 bits), L = longword (32 bits)
>> - */
>> -
>> -#define CF_LE_W(v) le16_to_cpu(v)
>> -#define CF_LE_L(v) le32_to_cpu(v)
>> -#define CT_LE_W(v) cpu_to_le16(v)
>> -#define CT_LE_L(v) cpu_to_le32(v)
>
> Personally I think this patch makes code readable. But please don't
> remove Cx_LE_x macros. Cx_LE_x is used from dosfsck.

Then the macros should be put in dosfsck, which is not
part of the kernel.

> -	logical_sector_size =
> -		le16_to_cpu(get_unaligned((unsigned short *) &b->sector_size));
> +	logical_sector_size = le16_to_cpu(get_unaligned((u16*)&b->sector_size));

I notice lots of casts in the code. It would be better to
use the correct types to begin with.

Re: [patch] fat/msdos/vfat crud removal

[OGAWA Hirofumi]

"Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

> OGAWA Hirofumi writes:
> > "Albert D. Cahalan" <acahalan@cs.uml.edu> writes:
> 
> >> - * Conversion from and to little-endian byte order. (no-op on i386/i486)
> >> - *
> >> - * Naming: Ca_b_c, where a: F = from, T = to, b: LE = little-endian,
> >> - * BE = big-endian, c: W = word (16 bits), L = longword (32 bits)
> >> - */
> >> -
> >> -#define CF_LE_W(v) le16_to_cpu(v)
> >> -#define CF_LE_L(v) le32_to_cpu(v)
> >> -#define CT_LE_W(v) cpu_to_le16(v)
> >> -#define CT_LE_L(v) cpu_to_le32(v)
> >
> > Personally I think this patch makes code readable. But please don't
> > remove Cx_LE_x macros. Cx_LE_x is used from dosfsck.
> 
> Then the macros should be put in dosfsck, which is not
> part of the kernel.

Why do we throw away backward compatible?

> > -	logical_sector_size =
> > -		le16_to_cpu(get_unaligned((unsigned short *) &b->sector_size));
> > +	logical_sector_size = le16_to_cpu(get_unaligned((u16*)&b->sector_size));
> 
> I notice lots of casts in the code. It would be better to
> use the correct types to begin with.

No, this field is aliment problem.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] fat/msdos/vfat crud removal

[Albert D. Cahalan]

OGAWA Hirofumi writes:
> "Albert D. Cahalan" <acahalan@cs.uml.edu> writes:
>> OGAWA Hirofumi writes:
>>> "Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

>>>> - * Conversion from and to little-endian byte order. (no-op on i386/i486)
>>>> - *
>>>> - * Naming: Ca_b_c, where a: F = from, T = to, b: LE = little-endian,
>>>> - * BE = big-endian, c: W = word (16 bits), L = longword (32 bits)
>>>> - */
>>>> -
>>>> -#define CF_LE_W(v) le16_to_cpu(v)
>>>> -#define CF_LE_L(v) le32_to_cpu(v)
>>>> -#define CT_LE_W(v) cpu_to_le16(v)
>>>> -#define CT_LE_L(v) cpu_to_le32(v)
>>>
>>> Personally I think this patch makes code readable. But please don't
>>> remove Cx_LE_x macros. Cx_LE_x is used from dosfsck.
>>
>> Then the macros should be put in dosfsck, which is not
>> part of the kernel.
>
> Why do we throw away backward compatible?

1. app source code isn't supposed to use raw kernel headers
2. existing executables are not affected
3. the 2.5.xx series has already broken much more
4. it's crud for the kernel; it's crud for user code
5. the kernel shouldn't contain misc. user app code

>>> -	logical_sector_size =
>>> -		le16_to_cpu(get_unaligned((unsigned short *) &b->sector_size));
>>> +	logical_sector_size = le16_to_cpu(get_unaligned((u16*)&b->sector_size));
>>
>> I notice lots of casts in the code. It would be better to
>> use the correct types to begin with.
>
> No, this field is aliment problem.

Use the packed attribute on the struct, along with
the right types. I don't think you need get_unaligned
with a packed struct, because gcc will know that it
needs to emit code for unaligned data.

At the very least you can get rid of the cast.

Before:
logical_sector_size = le16_to_cpu(get_unaligned((u16*)&b->sector_size));

After:
logical_sector_size = le16_to_cpu(b->sector_size);

The new struct:

/* Note the end: __attribute__ ((packed)) */
struct fat_boot_sector {
        char    ignored[3];     /* Boot strap short or near jump */
        __u64   system_id;      /* Name - can be used to special case
                                   partition manager volumes */
        __u16   sector_size;    /* bytes per logical sector */
        __u8    cluster_size;   /* sectors/cluster */
        __u16   reserved;       /* reserved sectors */
        __u8    fats;           /* number of FATs */
        __u16   dir_entries;    /* root directory entries */
        __u16   sectors;        /* number of sectors */
        __u8    media;          /* media code */
        __u16   fat_length;     /* sectors/FAT */
        __u16   secs_track;     /* sectors per track */
        __u16   heads;          /* number of heads */
        __u32   hidden;         /* hidden sectors (unused) */
        __u32   total_sect;     /* number of sectors (if sectors == 0) */

        /* The following fields are only used by FAT32 */
        __u32   fat32_length;   /* sectors/FAT */
        __u16   flags;          /* bit 8: fat mirroring, low 4: active fat */
        __u16   version;        /* major, minor filesystem version */
        __u32   root_cluster;   /* first cluster in root directory */
        __u16   info_sector;    /* filesystem info sector */
        __u16   backup_boot;    /* backup boot sector */
        __u16   reserved2[6];   /* Unused */
} __attribute__ ((packed)) ;

Re: [patch] fat/msdos/vfat crud removal

[OGAWA Hirofumi]

"Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

> OGAWA Hirofumi writes:
> > "Albert D. Cahalan" <acahalan@cs.uml.edu> writes:
> >> OGAWA Hirofumi writes:
> >>> "Albert D. Cahalan" <acahalan@cs.uml.edu> writes:
> 
> >>>> - * Conversion from and to little-endian byte order. (no-op on i386/i486)
> >>>> - *
> >>>> - * Naming: Ca_b_c, where a: F = from, T = to, b: LE = little-endian,
> >>>> - * BE = big-endian, c: W = word (16 bits), L = longword (32 bits)
> >>>> - */
> >>>> -
> >>>> -#define CF_LE_W(v) le16_to_cpu(v)
> >>>> -#define CF_LE_L(v) le32_to_cpu(v)
> >>>> -#define CT_LE_W(v) cpu_to_le16(v)
> >>>> -#define CT_LE_L(v) cpu_to_le32(v)
> >>>
> >>> Personally I think this patch makes code readable. But please don't
> >>> remove Cx_LE_x macros. Cx_LE_x is used from dosfsck.
> >>
> >> Then the macros should be put in dosfsck, which is not
> >> part of the kernel.
> >
> > Why do we throw away backward compatible?
> 
> 1. app source code isn't supposed to use raw kernel headers
> 2. existing executables are not affected
> 3. the 2.5.xx series has already broken much more
> 4. it's crud for the kernel; it's crud for user code
> 5. the kernel shouldn't contain misc. user app code

Why is there __KERNEL__ macro?

> Use the packed attribute on the struct, along with
> the right types. I don't think you need get_unaligned
> with a packed struct, because gcc will know that it
> needs to emit code for unaligned data.

OK. Please send patch.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] fat/msdos/vfat crud removal

[Albert D. Cahalan]

OGAWA Hirofumi writes:
> "Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

>> 1. app source code isn't supposed to use raw kernel headers
>> 2. existing executables are not affected
>> 3. the 2.5.xx series has already broken much more
>> 4. it's crud for the kernel; it's crud for user code
>> 5. the kernel shouldn't contain misc. user app code
>
> Why is there __KERNEL__ macro?

Long ago, it was considered OK to use the kernel headers
in app code. This is the case with Linux 2.0 and libc 5.
(it used to be OK to symlink /usr/include/linux into an
unmodified copy of the Linux kernel source)

There has been a weak effort to avoid breaking libc 5.

Using __KERNEL__ might make it easier to provide cleaned
headers for user code.

There has been talk of removing __KERNEL__ usage from
some of the header files.

Re: [patch] fat/msdos/vfat crud removal

[OGAWA Hirofumi]

"Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

> Long ago, it was considered OK to use the kernel headers
> in app code. This is the case with Linux 2.0 and libc 5.
> (it used to be OK to symlink /usr/include/linux into an
> unmodified copy of the Linux kernel source)
> 
> There has been a weak effort to avoid breaking libc 5.
> 
> Using __KERNEL__ might make it easier to provide cleaned
> headers for user code.
> 
> There has been talk of removing __KERNEL__ usage from
> some of the header files.

So, are you going to remove __KERNEL__ stuff, although the program for
linux uses it? And are you going to fix program using it?

I don't want to do.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] fat/msdos/vfat crud removal

[Dave Jones]

On Sun, Jun 09, 2002 at 03:09:44AM -0400, Albert D. Cahalan wrote:

 > There has been talk of removing __KERNEL__ usage from
 > some of the header files.

Where? If anything we need to increase __KERNEL__ usage in headers.
We export far too much crap which makes no sense to userspace.

        Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [patch] fat/msdos/vfat crud removal

[Eric W. Biederman]

Dave Jones <davej@suse.de> writes:

> On Sun, Jun 09, 2002 at 03:09:44AM -0400, Albert D. Cahalan wrote:
> 
>  > There has been talk of removing __KERNEL__ usage from
>  > some of the header files.
> 
> Where? If anything we need to increase __KERNEL__ usage in headers.
> We export far too much crap which makes no sense to userspace.

So we should just remove __KERNEL__ altogether.  And say with 2.5.x
nothing is exported.  Which pretty much has been the official policy
since user space started using glibc.

#include <linux/*>
and 
#include <asm/*>
are no longer supported.

Eric

Re: [patch] fat/msdos/vfat crud removal

[H. Peter Anvin]

Followup to:  <m18z5owd9f.fsf@frodo.biederman.org>
By author:    ebiederm@xmission.com (Eric W. Biederman)
In newsgroup: linux.dev.kernel
>
> Dave Jones <davej@suse.de> writes:
> 
> > On Sun, Jun 09, 2002 at 03:09:44AM -0400, Albert D. Cahalan wrote:
> > 
> >  > There has been talk of removing __KERNEL__ usage from
> >  > some of the header files.
> > 
> > Where? If anything we need to increase __KERNEL__ usage in headers.
> > We export far too much crap which makes no sense to userspace.
> 
> So we should just remove __KERNEL__ altogether.  And say with 2.5.x
> nothing is exported.  Which pretty much has been the official policy
> since user space started using glibc.
> 
> #include <linux/*>
> and 
> #include <asm/*>
> are no longer supported.
> 

In theory, perhaps.  There is plenty that just really can't be done
that way, especially stuff which deals with ioctls and their
structures.

It makes more sense to constrain what is exported to a minimum, but
actually have it be usable.

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt	<amsp@zytor.com>

Re: [patch] fat/msdos/vfat crud removal

[Thunder from the hill]

Hi,

On 9 Jun 2002, Eric W. Biederman wrote:
> #include <linux/*>
> and 
> #include <asm/*>
> are no longer supported.

Stop! The reason for _some_ includes there is actually to keep some 
definitions in sync with the kernel, e.g. errno values! Stopping them 
altogether is a Really Bad Thing[tm], IMO, since it means users will have 
to get a new glibc with almost every kernel they have (don't tell me we 
don't change much!).

I'm against it.

Regards,
Thunder
-- 
German attitude becoming        |	Thunder from the hill at ngforever
rightaway popular:		|
       "Get outa my way,  	|	free inhabitant not directly
    for I got a mobile phone!"	|	belonging anywhere

Re: [patch] fat/msdos/vfat crud removal

[David Woodhouse]

thunder@ngforever.de said:
>  Stop! The reason for _some_ includes there is actually to keep some
> definitions in sync with the kernel, e.g. errno values! Stopping them
> altogether is a Really Bad Thing[tm], IMO, since it means users will
> have  to get a new glibc with almost every kernel they have (don't
> tell me we  don't change much!).

Er, no. If you randomly reassign errno values, the world breaks.
Don't even contemplate it.

The _only_ thing that userspace could possibly pick from the kernel headers 
is the ABI. But if the ABI changes, that _must_ be a carefully-considered 
thing.

To that end, we should put '#ifndef __KERNEL__ #error' into all kernel
headers, and C libraries should maintain a _separate_ set of headers which
contain only the ABI definitions and are suitable for userspace. I believe 
dietlibc already does this, and recent Red Hat distributions contain a 
'glibc-kernheaders' package with a slightly-sanitised version of kernel 
headers, which should become more sanitised over time.

--
dwmw2

Re: [patch] fat/msdos/vfat crud removal

[Thunder from the hill]

Hi,

On Sun, 9 Jun 2002, David Woodhouse wrote:
> Er, no. If you randomly reassign errno values, the world breaks.
> Don't even contemplate it.

I meant adding. Not just errno, even PF_..., etc.

> To that end, we should put '#ifndef __KERNEL__ #error' into all kernel
> headers, and C libraries should maintain a _separate_ set of headers which
> contain only the ABI definitions and are suitable for userspace. I believe 
> dietlibc already does this, and recent Red Hat distributions contain a 
> 'glibc-kernheaders' package with a slightly-sanitised version of kernel 
> headers, which should become more sanitised over time.

I wouldn't call dietlibc an HighEnd open end API.

Regards,
Thunder
--
German attitude becoming        |       Thunder from the hill at ngforever 
rightaway popular:              |
       "Get outa my way,  	|	free inhabitant not directly
    for I got a mobile phone!"	|	belonging anywhere

Re: [patch] fat/msdos/vfat crud removal

[Eric W. Biederman]

Thunder from the hill <thunder@ngforever.de> writes:

> Hi,
> 
> On Sun, 9 Jun 2002, David Woodhouse wrote:
> > Er, no. If you randomly reassign errno values, the world breaks.
> > Don't even contemplate it.
> 
> I meant adding. Not just errno, even PF_..., etc.
> 
> > To that end, we should put '#ifndef __KERNEL__ #error' into all kernel
> > headers, and C libraries should maintain a _separate_ set of headers which
> > contain only the ABI definitions and are suitable for userspace. I believe 
> > dietlibc already does this, and recent Red Hat distributions contain a 
> > 'glibc-kernheaders' package with a slightly-sanitised version of kernel 
> > headers, which should become more sanitised over time.
> 
> I wouldn't call dietlibc an HighEnd open end API.

All linux libc's do this.  glibc, dietlibc, and uclibc.

Beyond this if you really object you can come up with a set of header
that just describe the kernel/user space ABI, and build them so either
the kernel or user space can use them.  And then this ABI-headers
package can be used to hold the common definitions.

Until someone builds a kernel-abi-headers package everyone will do it
by copying the appropriate headers periodically.

Eric

Re: [patch] fat/msdos/vfat crud removal

[Albert D. Cahalan]

Thunder from the h writes:
> On 9 Jun 2002, Eric W. Biederman wrote:

>> #include <linux/*>
>> and 
>> #include <asm/*>
>> are no longer supported.

Try "are no longer supplied by raw kernel source" instead.
They damn well better exist, cleaned up for non-kernel use.

> Stop! The reason for _some_ includes there is actually to keep some 
> definitions in sync with the kernel, e.g. errno values! Stopping them 
> altogether is a Really Bad Thing[tm], IMO, since it means users will have 
> to get a new glibc with almost every kernel they have (don't tell me we 
> don't change much!).
>
> I'm against it.

Too late. You're WAY too late. This is on a Debian box:

$ /bin/ls -ldog /usr/include/linux
drwxr-xr-x   11 root        28672 Jun  4 19:41 /usr/include/linux

As you can see, a kernel upgrade would do nothing to
change the headers in /usr/include/linux.

Re: [patch] fat/msdos/vfat crud removal

[Eric W. Biederman]

"Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

> Thunder from the h writes:
> > On 9 Jun 2002, Eric W. Biederman wrote:
> 
> >> #include <linux/*>
> >> and 
> >> #include <asm/*>
> >> are no longer supported.
> 
> Try "are no longer supplied by raw kernel source" instead.
> They damn well better exist, cleaned up for non-kernel use.

And user space should gradually be fixed from using them.  In almost
every case there are more appropriate headers to use.  Basically
keeping the /usr/include/linux and /usr/include/asm  directories is a
crutch to allow a slow user space transition.

Actually by now most applications have been fixed and do not use
them.  The policy has been in place for several years now.

Eric

Re: [patch] fat/msdos/vfat crud removal

[Jamie Lokier]

Eric W. Biederman wrote:
> Actually by now most applications have been fixed and do not use
> them.  The policy has been in place for several years now.

I like this policy and understand how to use it, except...

Once upon a time I wrote a program which used O_NOFOLLOW, before Glibc
had support for that flag.

It had to read the kernel headers, as this macro is an
architecture-dependent flag, and I did not want to write a program that
was so non-portable it would only compile on some architectures.

Even if I'd copied all the definitions for all architectures out of the
kernel, that wouldn't do: the program wouldn't compile on architectures
added later, or ones that aren't part of the standard distribution.

So to keep the program relatively portable, it searched for definitions
of O_NOFOLLOW in the kernel headers.  (It was a Glibc/kernel conflict
nightmare).

Please can you suggest how I should write this sort of code, the next
time it occurs?

thanks,
-- Jamie

Re: [patch] fat/msdos/vfat crud removal

[Eric W. Biederman]

Jamie Lokier <lk@tantalophile.demon.co.uk> writes:

> Eric W. Biederman wrote:
> > Actually by now most applications have been fixed and do not use
> > them.  The policy has been in place for several years now.
> 
> I like this policy and understand how to use it, except...
 
First early adopters of any feature have a challenge.  

> Once upon a time I wrote a program which used O_NOFOLLOW, before Glibc
> had support for that flag.
> 
> It had to read the kernel headers, as this macro is an
> architecture-dependent flag, and I did not want to write a program that
> was so non-portable it would only compile on some architectures.
> 
> Even if I'd copied all the definitions for all architectures out of the
> kernel, that wouldn't do: the program wouldn't compile on architectures
> added later, or ones that aren't part of the standard distribution.
> 
> So to keep the program relatively portable, it searched for definitions
> of O_NOFOLLOW in the kernel headers.  (It was a Glibc/kernel conflict
> nightmare).
> 
> Please can you suggest how I should write this sort of code, the next
> time it occurs?

Hmm.  I think I would do:
/* use the standard open includes */
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#ifndef O_NOFOLLOW
#ifdef __i386__
#define O_NOFOLLOW  0400000
#else
#error I must have O_NOFOLLOW fix glibc
#endif
#endif

Or if it really didn't matter much:
#ifndef O_NOFOLLOW
#define O_NOFOLLOW 0
#endif

But even beyond that it makes a lot of sense to have autoconf add
a test for O_NOFOLLOW, and do something appropriate if it isn't
defined, or supported.  Otherwise your code gets brittle anyway. 

Where generic API extensions end up in the future is pretty well
defined, so you don't have to guess where libc will put them.

In most cases the kernel header rule only applies to pure linux
specific interfaces where the numbers don't change between
architectures, and to interfaces that are specific to a small subset
of programs, (device specific code like hdparm).   

But even using kernel headers doesn't help on current systems,
as you should have headers that correspond to the version of the
kernel your libc was compiled against.  So if libc didn't support
O_NOFOLLOW it is quite unlikely that user space would in any form.

Eric

Re: [patch] fat/msdos/vfat crud removal

[Thunder from the hill]

Hi,

On 12 Jun 2002, Eric W. Biederman wrote:
> > Eric W. Biederman wrote:
> > > Actually by now most applications have been fixed and do not use
> > > them.  The policy has been in place for several years now.

That means the possible policy of

#include <stdio.h>
#include <linux/version.h>

void exit(int code);

int main(void) {
    if (LINUX_VERSION_CODE < KERNEL_VERSION(2,4,0)) {
        fprintf(stderr, "Your kernel is too old!\n");
        exit(-127);
    }

    printf("Kernel version OK.\n");

    exit(0);
}

is impossible now after all, which is good, I think, because who said that 
the headers have to come from the _real_ _configured_ kernel? (It was way 
too crappy.)

Regards,
Thunder
-- 
German attitude becoming        |	Thunder from the hill at ngforever
rightaway popular:		|
       "Get outa my way,  	|	free inhabitant not directly
    for I got a mobile phone!"	|	belonging anywhere

Re: [patch] fat/msdos/vfat crud removal

[OGAWA Hirofumi]

"Albert D. Cahalan" <acahalan@cs.uml.edu> writes:

> Use the packed attribute on the struct, along with
> the right types. I don't think you need get_unaligned
> with a packed struct, because gcc will know that it
> needs to emit code for unaligned data.
> 
> At the very least you can get rid of the cast.
> 
> Before:
> logical_sector_size = le16_to_cpu(get_unaligned((u16*)&b->sector_size));
> 
> After:
> logical_sector_size = le16_to_cpu(b->sector_size);
> 
> The new struct:
> 
> /* Note the end: __attribute__ ((packed)) */
> struct fat_boot_sector {
>         char    ignored[3];     /* Boot strap short or near jump */
>         __u64   system_id;      /* Name - can be used to special case
>                                    partition manager volumes */
>         __u16   sector_size;    /* bytes per logical sector */
>         __u8    cluster_size;   /* sectors/cluster */
>         __u16   reserved;       /* reserved sectors */
>         __u8    fats;           /* number of FATs */
>         __u16   dir_entries;    /* root directory entries */
>         __u16   sectors;        /* number of sectors */
>         __u8    media;          /* media code */
>         __u16   fat_length;     /* sectors/FAT */
>         __u16   secs_track;     /* sectors per track */
>         __u16   heads;          /* number of heads */
>         __u32   hidden;         /* hidden sectors (unused) */
>         __u32   total_sect;     /* number of sectors (if sectors == 0) */
> 
>         /* The following fields are only used by FAT32 */
>         __u32   fat32_length;   /* sectors/FAT */
>         __u16   flags;          /* bit 8: fat mirroring, low 4: active fat */
>         __u16   version;        /* major, minor filesystem version */
>         __u32   root_cluster;   /* first cluster in root directory */
>         __u16   info_sector;    /* filesystem info sector */
>         __u16   backup_boot;    /* backup boot sector */
>         __u16   reserved2[6];   /* Unused */
> } __attribute__ ((packed)) ;

BTW, is this safe on other of i386 (ex. mips etc.)? I'm not sure.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] fat/msdos/vfat crud removal

[Martin Dalecki]

Albert D. Cahalan wrote:

> The new struct:
> 
> /* Note the end: __attribute__ ((packed)) */
> struct fat_boot_sector {
>         char    ignored[3];     /* Boot strap short or near jump */
>         __u64   system_id;      /* Name - can be used to special case
>                                    partition manager volumes */
....
>         __u16   info_sector;    /* filesystem info sector */
>         __u16   backup_boot;    /* backup boot sector */
>         __u16   reserved2[6];   /* Unused */
> } __attribute__ ((packed)) ;


And don't use __uXX but just uXX for bit field integer types in
sturctures, which are supposed to be only used by the kernel.