Phillip Lougher <[EMAIL PROTECTED]> wrote:
>
>
Please don't send multiple patches with the same Subject:. Choose nice,
meaningful Subject:s for each patch. And include the relevant changelog
details within the email for each patch rather than in patch 1/N. See
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt and
http://linux.yyz.us/patch-format.html.
> @@ -0,0 +1,439 @@
[lots of comments from patch 1/2 are applicable here]
> +#define SQUASHFS_MAX_FILE_SIZE ((long long) 1 << \
> + (SQUASHFS_MAX_FILE_SIZE_LOG - 1))
1LL would suit here. Of a cast to loff_t.
> +typedef unsigned int squashfs_block;
> +typedef long long squashfs_inode;
squashfs_block_t and squashfs_inode_t, please. If one must use typedefs...
> +typedef struct squashfs_super_block {
> + unsigned int s_magic;
> + unsigned int inodes;
> + unsigned int bytes_used;
> + unsigned int uid_start;
> + unsigned int guid_start;
> + unsigned int inode_table_start;
> + unsigned int directory_table_start;
> + unsigned int s_major:16;
> + unsigned int s_minor:16;
> + unsigned int block_size_1:16;
> + unsigned int block_log:16;
> + unsigned int flags:8;
> + unsigned int no_uids:8;
> + unsigned int no_guids:8;
> + unsigned int mkfs_time /* time of filesystem creation */;
> + squashfs_inode root_inode;
> + unsigned int block_size;
> + unsigned int fragments;
> + unsigned int fragment_table_start;
> +} __attribute__ ((packed)) squashfs_super_block;
Whoa. Tons of bitfields in this file. Are these on-disk data structures?
If so, that's a problem for portability between architectures and possibly
compiler versions. It also introduces locking complexity.
if they're in-core data structures then the bitfields are probably slower than
using `int', as well.
> +typedef struct {
> + unsigned int inode_type:4;
> + unsigned int mode:12; /* protection */
> + unsigned int uid:8; /* index into uid table */
> + unsigned int guid:8; /* index into guid table */
> +} __attribute__ ((packed)) squashfs_base_inode_header;
See, if one CUP is modifying `inode_type' while another CPU is modifying
`mode', this struct can get trashed.
> +/*
> + * macros to convert each packed bitfield structure from little endian to big
> + * endian and vice versa. These are needed when creating or using a
> filesystem
> + * on a machine with different byte ordering to the target architecture.
> + *
> + */
hmm, OK.. Tell us more?
> + * bitfields and different bitfield placing conventions on differing
> + * architectures
> + */
> +
> +#include <asm/byteorder.h>
> +
> +#ifdef __BIG_ENDIAN
> + /* convert from little endian to big endian */
> +#define SQUASHFS_SWAP(value, p, pos, tbits) _SQUASHFS_SWAP(value, p, pos, \
> + tbits, b_pos)
> +#else
> + /* convert from big endian to little endian */
> +#define SQUASHFS_SWAP(value, p, pos, tbits) _SQUASHFS_SWAP(value, p, pos, \
> + tbits, 64 - tbits - b_pos)
> +#endif
> +
> +#define _SQUASHFS_SWAP(value, p, pos, tbits, SHIFT) {\
> + int bits;\
> + int b_pos = pos % 8;\
> + unsigned long long val = 0;\
> + unsigned char *s = (unsigned char *)p + (pos / 8);\
> + unsigned char *d = ((unsigned char *) &val) + 7;\
> + for(bits = 0; bits < (tbits + b_pos); bits += 8) \
> + *d-- = *s++;\
> + value = (val >> (SHIFT))/* & ((1 << tbits) - 1)*/;\
> +}
Can the standard leXX_to_cpu() helpers not be used here?
> +#include <linux/squashfs_fs.h>
> +
> +typedef struct {
> + unsigned int block;
> + int length;
> + unsigned int next_index;
> + char *data;
> + } squashfs_cache;
Whitespace inconsistency (column 1 for the closing brace is standard)
--- linux-2.6.11.3/init/do_mounts_rd.c 2005-03-13 06:44:30.000000000 +0000
+++ linux-2.6.11.3-squashfs/init/do_mounts_rd.c 2005-03-14 00:53:28.092559728
+0000
Your changelog didn't mention that squashfs interacts with the boot
process. That's the sort of thing which is nice to tell people about.
> +SQUASHFS FILESYSTEM
> +P: Phillip Lougher
> +M: [EMAIL PROTECTED]
> +W: http://squashfs.sourceforge.net
> +L: [EMAIL PROTECTED]
> +S: Maintained
> +
Lots of little comments, but I have no fundamental problems with the
patches as long as the bitfield issue is shown to be a non-issue.
Please respin the patches and unless someone else sees a showstopper I'll
merge them into -mm for further testing and review, thanks.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
