2002-06-09 02:47:31

by Albert D. Cahalan

[permalink] [raw]
Subject: [patch] fat/msdos/vfat crud removal


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


2002-06-09 04:25:09

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

"Albert D. Cahalan" <[email protected]> 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 <[email protected]>

2002-06-09 04:32:57

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

Sorry, my previous miss email.

"Albert D. Cahalan" <[email protected]> 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 <[email protected]>

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)

/*

2002-06-09 04:46:11

by Albert D. Cahalan

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

OGAWA Hirofumi writes:
> "Albert D. Cahalan" <[email protected]> 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.

2002-06-09 05:07:46

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

"Albert D. Cahalan" <[email protected]> writes:

> OGAWA Hirofumi writes:
> > "Albert D. Cahalan" <[email protected]> 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 <[email protected]>

2002-06-09 06:03:18

by Albert D. Cahalan

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

OGAWA Hirofumi writes:
> "Albert D. Cahalan" <[email protected]> writes:
>> OGAWA Hirofumi writes:
>>> "Albert D. Cahalan" <[email protected]> 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)) ;

2002-06-09 06:32:41

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

"Albert D. Cahalan" <[email protected]> writes:

> OGAWA Hirofumi writes:
> > "Albert D. Cahalan" <[email protected]> writes:
> >> OGAWA Hirofumi writes:
> >>> "Albert D. Cahalan" <[email protected]> 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 <[email protected]>

2002-06-09 07:09:45

by Albert D. Cahalan

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

OGAWA Hirofumi writes:
> "Albert D. Cahalan" <[email protected]> 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.

2002-06-09 07:50:22

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

"Albert D. Cahalan" <[email protected]> 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 <[email protected]>

2002-06-09 09:00:46

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

"Albert D. Cahalan" <[email protected]> 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 <[email protected]>

2002-06-09 09:46:41

by Dave Jones

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

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

2002-06-09 14:57:24

by Martin Dalecki

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

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.


2002-06-09 18:05:19

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

Dave Jones <[email protected]> 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

2002-06-09 18:50:06

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

Followup to: <[email protected]>
By author: [email protected] (Eric W. Biederman)
In newsgroup: linux.dev.kernel
>
> Dave Jones <[email protected]> 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
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt <[email protected]>

2002-06-09 19:00:46

by Thunder from the hill

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

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

2002-06-09 19:34:05

by David Woodhouse

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal


[email protected] 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


2002-06-09 20:20:24

by Thunder from the hill

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

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

2002-06-09 21:04:28

by Albert D. Cahalan

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

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.

2002-06-10 14:03:30

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

Thunder from the hill <[email protected]> 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

2002-06-10 14:07:33

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

"Albert D. Cahalan" <[email protected]> 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

2002-06-11 16:50:22

by Jamie Lokier

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

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

2002-06-12 07:00:40

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

Jamie Lokier <[email protected]> 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

2002-06-12 11:48:27

by Thunder from the hill

[permalink] [raw]
Subject: Re: [patch] fat/msdos/vfat crud removal

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