Phillip Lougher <[EMAIL PROTECTED]> wrote:
>
> Please apply the following two patches which adds SquashFS to the
> kernel.
> +
> +#include <linux/types.h>
> +#include <linux/squashfs_fs.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/smp_lock.h>
> +#include <linux/slab.h>
> +#include <linux/squashfs_fs_sb.h>
> +#include <linux/squashfs_fs_i.h>
> +#include <linux/buffer_head.h>
> +#include <linux/vfs.h>
> +#include <linux/init.h>
> +#include <linux/dcache.h>
> +#include <asm/uaccess.h>
> +#include <linux/wait.h>
> +#include <asm/semaphore.h>
> +#include <linux/zlib.h>
> +#include <linux/blkdev.h>
> +#include <linux/vmalloc.h>
> +#include "squashfs.h"
> +
We normally put aam includes after linux includes:
#include <linux/a.h>
#include <linux/b.h>
#include <asm/a.h>
#Include <asm/b.h>
> +
> +DECLARE_MUTEX(read_data_mutex);
> +
Does this need to have global scope? If so, it needs a less generic name.
`squashfs_read_data_mutex' would suit.
> +static struct file_system_type squashfs_fs_type = {
> + .owner = THIS_MODULE,
> + .name = "squashfs",
> + .get_sb = squashfs_get_sb,
> + .kill_sb = kill_block_super,
> + .fs_flags = FS_REQUIRES_DEV
> + };
> +
The final brace should go in column 1.
> +
> +static struct buffer_head *get_block_length(struct super_block *s,
> + int *cur_index, int *offset, int *c_byte)
> +{
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;
s_fs_info has type void*. Hence there is no need to typecast when
assigning pointers to or from it. In fact it is a little harmful to do so.
Please search both your patches for all occurrences of s_fs_info and remove
the typecasts. There are many.
> + unsigned short temp;
> + struct buffer_head *bh;
> +
> + if (!(bh = sb_bread(s, *cur_index)))
> + return NULL;
> +
> + if (msBlk->devblksize - *offset == 1) {
> + if (msBlk->swap)
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + (bh->b_data + *offset));
> + else
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + (bh->b_data + *offset));
All this typecasting looks nasty. Is there a nicer way? Perhaps using a
temporary variable?
Is this code endian-safe?
> + if (msBlk->swap)
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + bh->b_data);
> + else
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + bh->b_data);
> + *c_byte = temp;
> + *offset = 1;
> + } else {
> + if (msBlk->swap) {
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + (bh->b_data + *offset));
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + (bh->b_data + *offset + 1));
> + } else {
> + ((unsigned char *) &temp)[0] = *((unsigned char *)
> + (bh->b_data + *offset));
> + ((unsigned char *) &temp)[1] = *((unsigned char *)
> + (bh->b_data + *offset + 1));
> + }
Ditto.
> +
> + if (SQUASHFS_CHECK_DATA(msBlk->sBlk.flags)) {
> + if (*offset == msBlk->devblksize) {
> + brelse(bh);
> + if (!(bh = sb_bread(s, ++(*cur_index))))
> + return NULL;
> + *offset = 0;
> + }
> + if (*((unsigned char *) (bh->b_data + *offset)) !=
> + SQUASHFS_MARKER_BYTE) {
> + ERROR("Metadata block marker corrupt @ %x\n",
> + *cur_index);
> + brelse(bh);
> + return NULL;
Multiple return statements per function are a maintainability problem,
especially if some of them are deep inside that function's logic. The old
`goto out' is preferred.
(Imagine what would happen if you later wanted to change this function to
kmalloc a bit of temp storage and you don't want it to leak).
> + }
> + (*offset) ++;
whitespace.
> +unsigned int squashfs_read_data(struct super_block *s, char *buffer,
> + unsigned int index, unsigned int length,
> + unsigned int *next_index)
> +{
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;
> + struct buffer_head *bh[((SQUASHFS_FILE_MAX_SIZE - 1) >>
> + msBlk->devblksize_log2) + 2];
Dynamically sized local storage. Deliberate? What is the upper bound on
its size?
> +block_release:
> + while (--b >= 0) brelse(bh[b]);
while (--b >= 0)
brelse(bh[b]);
please.
> +
> + if (n == 0) {
> + wait_queue_t wait;
> +
> + init_waitqueue_entry(&wait, current);
> + add_wait_queue(&msBlk->waitq, &wait);
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + up(&msBlk->block_cache_mutex);
> + schedule();
> + set_current_state(TASK_RUNNING);
> + remove_wait_queue(&msBlk->waitq, &wait);
> + continue;
> + }
- Use DECLARE_WAITQUEUE (or DEFINE_WAIT)
- schedule() always returns in state TASK_RUNNING.
> + msBlk->next_cache = (i + 1) % SQUASHFS_CACHED_BLKS;
> +
> + if (msBlk->block_cache[i].block ==
> + SQUASHFS_INVALID_BLK) {
> + if (!(msBlk->block_cache[i].data =
> + (unsigned char *)
> + kmalloc(SQUASHFS_METADATA_SIZE,
> + GFP_KERNEL))) {
kmalloc() returns void*, hence no cast is needed. Please search both
patches for all instances of kmalloc(), remove the typecasts.
> + if (n == 0) {
> + wait_queue_t wait;
> +
> + init_waitqueue_entry(&wait, current);
> + add_wait_queue(&msBlk->fragment_wait_queue,
> + &wait);
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + up(&msBlk->fragment_mutex);
> + schedule();
> + set_current_state(TASK_RUNNING);
> + remove_wait_queue(&msBlk->fragment_wait_queue,
> + &wait);
> + continue;
> + }
See above.
> +
> +static struct inode *squashfs_iget(struct super_block *s, squashfs_inode
> inode)
> +{
> + struct inode *i;
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + unsigned int block = SQUASHFS_INODE_BLK(inode) +
> + sBlk->inode_table_start;
> + unsigned int offset = SQUASHFS_INODE_OFFSET(inode);
> + unsigned int ino = SQUASHFS_MK_VFS_INODE(block
> + - sBlk->inode_table_start, offset);
> + unsigned int next_block, next_offset;
> + squashfs_base_inode_header inodeb;
How much stack space is being used here? Perhaps you should run
scripts/checkstack.pl across the whole thing.
> + TRACE("Entered squashfs_iget\n");
> +
> + if (msBlk->swap) {
> + squashfs_base_inode_header sinodeb;
> +
More stack
> +
> + switch(inodeb.inode_type) {
> + case SQUASHFS_FILE_TYPE: {
> + squashfs_reg_inode_header inodep;
> + unsigned int frag_blk, frag_size;
And more
> + case SQUASHFS_DIR_TYPE: {
> + squashfs_dir_inode_header inodep;
And more!
> + if (msBlk->swap) {
> + squashfs_dir_inode_header sinodep;
> + }
> + case SQUASHFS_LDIR_TYPE: {
> + squashfs_ldir_inode_header inodep;
> + }
> + case SQUASHFS_SYMLINK_TYPE: {
> + squashfs_symlink_inode_header inodep;
> + case SQUASHFS_BLKDEV_TYPE:
> + case SQUASHFS_CHRDEV_TYPE: {
> + squashfs_dev_inode_header inodep;
More.
> +static int squashfs_symlink_readpage(struct file *file, struct page *page)
> +{
> + struct inode *inode = page->mapping->host;
> + int index = page->index << PAGE_CACHE_SHIFT, length, bytes;
> + unsigned int block = SQUASHFS_I(inode)->start_block;
> + int offset = SQUASHFS_I(inode)->offset;
> + void *pageaddr = kmap(page);
> +
> + TRACE("Entered squashfs_symlink_readpage, page index %d, start block "
> + "%x, offset %x\n", page->index,
> + SQUASHFS_I(inode)->start_block,
> + SQUASHFS_I(inode)->offset);
> +
> + for (length = 0; length < index; length += bytes) {
> + if (!(bytes = squashfs_get_cached_block(inode->i_sb, NULL,
> + block, offset, PAGE_CACHE_SIZE, &block,
> + &offset))) {
> + ERROR("Unable to read symbolic link [%x:%x]\n", block,
> + offset);
> + goto skip_read;
> + }
> + }
> +
> + if (length != index) {
> + ERROR("(squashfs_symlink_readpage) length != index\n");
> + bytes = 0;
> + goto skip_read;
> + }
> +
> + bytes = (inode->i_size - length) > PAGE_CACHE_SIZE ? PAGE_CACHE_SIZE :
> + inode->i_size - length;
One should normally use i_size_read(), but I guess thi is OK on symlinks.
> +skip_read:
> + memset(pageaddr + bytes, 0, PAGE_CACHE_SIZE - bytes);
> + kunmap(page);
> + flush_dcache_page(page);
> + SetPageUptodate(page);
> + unlock_page(page);
> +
> + return 0;
> +}
There's no need to run flush_dcache_page() against a symlink - they cannot
be mmapped.
See if you can use kmap_atomic() here - kmap() is slow, theoretically
deadlocky and is deprecated.
> +static unsigned int read_blocklist(struct inode *inode, int index,
> + int readahead_blks, char *block_list,
> + unsigned short **block_p, unsigned int *bsize)
> +{
> ...
> +
> + if (msBlk->swap) {
> + unsigned char sblock_list[SIZE];
How large is `SIZE'?
That seems to be a risky choice of identifier. I guess it's so generic
that nobody else used it, so you're safe ;)
> +
> +static int squashfs_readpage(struct file *file, struct page *page)
> +{
> + struct inode *inode = page->mapping->host;
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)inode->i_sb->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + unsigned char block_list[SIZE];
> + unsigned int bsize, block, i = 0, bytes = 0, byte_offset = 0;
> + int index = page->index >> (sBlk->block_log - PAGE_CACHE_SHIFT);
> + void *pageaddr = kmap(page);
> + struct squashfs_fragment_cache *fragment = NULL;
> + char *data_ptr = msBlk->read_page;
> +
> + int mask = (1 << (sBlk->block_log - PAGE_CACHE_SHIFT)) - 1;
> + int start_index = page->index & ~mask;
> + int end_index = start_index | mask;
> +
> + TRACE("Entered squashfs_readpage, page index %x, start block %x\n",
> + (unsigned int) page->index,
> + SQUASHFS_I(inode)->start_block);
> +
> + if (page->index >= ((inode->i_size + PAGE_CACHE_SIZE - 1) >>
> + PAGE_CACHE_SHIFT)) {
> + goto skip_read;
> + }
This should use i_size_read(). Please review the patches for all instances
of open-coded i_size reads and writes.
> +
> + if (i == page->index) {
> + memcpy(pageaddr, data_ptr + byte_offset,
> + available_bytes);
> + memset(pageaddr + available_bytes, 0,
> + PAGE_CACHE_SIZE - available_bytes);
> + kunmap(page);
> + flush_dcache_page(page);
> + SetPageUptodate(page);
> + unlock_page(page);
> + } else if ((push_page =
> + grab_cache_page_nowait(page->mapping, i))) {
> + void *pageaddr = kmap(push_page);
> +
> + memcpy(pageaddr, data_ptr + byte_offset,
> + available_bytes);
> + memset(pageaddr + available_bytes, 0,
> + PAGE_CACHE_SIZE - available_bytes);
> + kunmap(push_page);
> + flush_dcache_page(push_page);
> + SetPageUptodate(push_page);
> + unlock_page(push_page);
> + page_cache_release(push_page);
kmap_atomic() can be used here.
> +static int squashfs_readpage4K(struct file *file, struct page *page)
> +{
> + struct inode *inode = page->mapping->host;
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)inode->i_sb->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + unsigned char block_list[SIZE];
> + unsigned int bsize, block, bytes = 0;
> + void *pageaddr = kmap(page);
Stack space?
> +
> +
> +static int get_dir_index_using_name(struct super_block *s, unsigned int
> + *next_block, unsigned int *next_offset,
> + unsigned int index_start,
> + unsigned int index_offset, int i_count,
> + const char *name, int size)
> +{
> + squashfs_sb_info *msBlk = (squashfs_sb_info *)s->s_fs_info;
> + squashfs_super_block *sBlk = &msBlk->sBlk;
> + int i, length = 0;
> + char buffer[sizeof(squashfs_dir_index) + SQUASHFS_NAME_LEN + 1];
> + squashfs_dir_index *index = (squashfs_dir_index *) buffer;
> + char str[SQUASHFS_NAME_LEN + 1];
> +
> + TRACE("Entered get_dir_index_using_name, i_count %d\n", i_count);
> +
> + strncpy(str, name, size);
> + str[size] = '\0';
Are you sure that this strncpy() is always safe?
> + dirs_read ++;
We don't put a space before the `++'. Please review both patches for this.
> +static struct inode *squashfs_alloc_inode(struct super_block *sb)
> +{
> + struct squashfs_inode_info *ei;
> + ei = (struct squashfs_inode_info *)
> + kmem_cache_alloc(squashfs_inode_cachep, SLAB_KERNEL);
kmem_cache_alloc() returns void*.
> +static void init_once(void * foo, kmem_cache_t * cachep, unsigned long flags)
> +{
> + struct squashfs_inode_info *ei = (struct squashfs_inode_info *) foo;
Unneeded typecast.
> +static int init_inodecache(void)
This can be __init.
-
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/
