LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Subject: [PATCH 01/16] Squashfs: inode operations
@ 2008-10-17 15:42 Phillip Lougher
  2008-10-17 16:53 ` Jörn Engel
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Phillip Lougher @ 2008-10-17 15:42 UTC (permalink / raw)
  To: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird


Signed-off-by: Phillip Lougher <phillip@lougher.demon.co.uk>
---
 fs/squashfs/inode.c |  318 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 318 insertions(+), 0 deletions(-)

diff --git a/fs/squashfs/inode.c b/fs/squashfs/inode.c
new file mode 100644
index 0000000..4782e1d
--- /dev/null
+++ b/fs/squashfs/inode.c
@@ -0,0 +1,318 @@
+/*
+ * Squashfs - a compressed read only filesystem for Linux
+ *
+ * Copyright (c) 2002, 2003, 2004, 2005, 2006, 2007, 2008
+ * Phillip Lougher <phillip@lougher.demon.co.uk>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * inode.c
+ */
+
+/*
+ * This file implements code to create and read inodes from disk.
+ *
+ * Inodes in Squashfs are identified by a 48-bit inode which encodes the
+ * location of the compressed metadata block containing the inode, and the byte
+ * offset into that block where the inode is placed (<block, offset>).
+ *
+ * To maximise compression there are different inodes for each file type
+ * (regular file, directory, device, etc.), the inode contents and length
+ * varying with the type.
+ *
+ * To further maximise compression, two types of regular file inode and
+ * directory inode are defined: inodes optimised for frequently occurring
+ * regular files and directories, and extended types where extra
+ * information has to be stored.
+ */
+
+#include <linux/fs.h>
+#include <linux/vfs.h>
+#include <linux/zlib.h>
+#include <linux/squashfs_fs.h>
+#include <linux/squashfs_fs_sb.h>
+#include <linux/squashfs_fs_i.h>
+
+#include "squashfs.h"
+
+/*
+ * Initialise VFS inode with the base inode information common to all
+ * Squashfs inode types.  Inodeb contains the unswapped base inode
+ * off disk.
+ */
+static int squashfs_new_inode(struct super_block *s, struct inode *i,
+				struct squashfs_base_inode *inodeb)
+{
+	if (squashfs_get_id(s, le16_to_cpu(inodeb->uid), &i->i_uid) == 0)
+		goto out;
+	if (squashfs_get_id(s, le16_to_cpu(inodeb->guid), &i->i_gid) == 0)
+		goto out;
+
+	i->i_ino = le32_to_cpu(inodeb->inode_number);
+	i->i_mtime.tv_sec = le32_to_cpu(inodeb->mtime);
+	i->i_atime.tv_sec = i->i_mtime.tv_sec;
+	i->i_ctime.tv_sec = i->i_mtime.tv_sec;
+	i->i_mode = le16_to_cpu(inodeb->mode);
+	i->i_size = 0;
+
+	return 1;
+
+out:
+	return 0;
+}
+
+
+struct inode *squashfs_iget(struct super_block *s, long long inode,
+				unsigned int inode_number)
+{
+	struct inode *i = iget_locked(s, inode_number);
+
+	TRACE("Entered squashfs_iget\n");
+
+	if (i && (i->i_state & I_NEW)) {
+		squashfs_read_inode(i, inode);
+		unlock_new_inode(i);
+	}
+
+	return i;
+}
+
+
+/*
+ * Initialise VFS inode by reading inode from inode table (compressed
+ * metadata).  The format and amount of data read depends on type.
+ */
+int squashfs_read_inode(struct inode *i, long long inode)
+{
+	struct super_block *s = i->i_sb;
+	struct squashfs_sb_info *msblk = s->s_fs_info;
+	long long block = SQUASHFS_INODE_BLK(inode) + msblk->inode_table_start;
+	unsigned int offset = SQUASHFS_INODE_OFFSET(inode);
+	long long next_block;
+	unsigned int next_offset;
+	int type;
+	union squashfs_inode id;
+	struct squashfs_base_inode *inodeb = &id.base;
+
+	TRACE("Entered squashfs_read_inode\n");
+
+	/*
+	 * Read inode base common to all inode types.
+	 */
+	if (!squashfs_read_metadata(s, inodeb, block, offset, sizeof(*inodeb),
+			&next_block, &next_offset))
+		goto failed_read;
+
+	if (squashfs_new_inode(s, i, inodeb) == 0)
+			goto failed_read;
+
+	type = le16_to_cpu(inodeb->inode_type);
+	switch (type) {
+	case SQUASHFS_FILE_TYPE: {
+		unsigned int frag_offset, frag_size, frag;
+		long long frag_blk;
+		struct squashfs_reg_inode *inodep = &id.reg;
+
+		if (!squashfs_read_metadata(s, inodep, block, offset,
+				sizeof(*inodep), &next_block, &next_offset))
+			goto failed_read;
+
+		frag = le32_to_cpu(inodep->fragment);
+		if (frag != SQUASHFS_INVALID_FRAG) {
+			frag_offset = le32_to_cpu(inodep->offset);
+			frag_size = get_fragment_location(s, frag, &frag_blk);
+			if (frag_size == 0)
+				goto failed_read;
+		} else {
+			frag_blk = SQUASHFS_INVALID_BLK;
+			frag_size = 0;
+			frag_offset = 0;
+		}
+
+		i->i_nlink = 1;
+		i->i_size = le32_to_cpu(inodep->file_size);
+		i->i_fop = &generic_ro_fops;
+		i->i_mode |= S_IFREG;
+		i->i_blocks = ((i->i_size - 1) >> 9) + 1;
+		SQUASHFS_I(i)->fragment_block = frag_blk;
+		SQUASHFS_I(i)->fragment_size = frag_size;
+		SQUASHFS_I(i)->fragment_offset = frag_offset;
+		SQUASHFS_I(i)->start_block = le32_to_cpu(inodep->start_block);
+		SQUASHFS_I(i)->block_list_start = next_block;
+		SQUASHFS_I(i)->offset = next_offset;
+		i->i_data.a_ops = &squashfs_aops;
+
+		TRACE("File inode %x:%x, start_block %llx, block_list_start "
+				"%llx, offset %x\n", SQUASHFS_INODE_BLK(inode),
+				offset, SQUASHFS_I(i)->start_block, next_block,
+				next_offset);
+		break;
+	}
+	case SQUASHFS_LREG_TYPE: {
+		unsigned int frag_offset, frag_size, frag;
+		long long frag_blk;
+		struct squashfs_lreg_inode *inodep = &id.lreg;
+
+		if (!squashfs_read_metadata(s, inodep, block, offset,
+				sizeof(*inodep), &next_block, &next_offset))
+			goto failed_read;
+
+		frag = le32_to_cpu(inodep->fragment);
+		if (frag != SQUASHFS_INVALID_FRAG) {
+			frag_offset = le32_to_cpu(inodep->offset);
+			frag_size = get_fragment_location(s, frag, &frag_blk);
+			if (frag_size == 0)
+				goto failed_read;
+		} else {
+			frag_blk = SQUASHFS_INVALID_BLK;
+			frag_size = 0;
+			frag_offset = 0;
+		}
+
+		i->i_nlink = le32_to_cpu(inodep->nlink);
+		i->i_size = le64_to_cpu(inodep->file_size);
+		i->i_fop = &generic_ro_fops;
+		i->i_mode |= S_IFREG;
+		i->i_blocks = ((i->i_size - le64_to_cpu(inodep->sparse) - 1)
+				>> 9) + 1;
+
+		SQUASHFS_I(i)->fragment_block = frag_blk;
+		SQUASHFS_I(i)->fragment_size = frag_size;
+		SQUASHFS_I(i)->fragment_offset = frag_offset;
+		SQUASHFS_I(i)->start_block = le64_to_cpu(inodep->start_block);
+		SQUASHFS_I(i)->block_list_start = next_block;
+		SQUASHFS_I(i)->offset = next_offset;
+		i->i_data.a_ops = &squashfs_aops;
+
+		TRACE("File inode %x:%x, start_block %llx, block_list_start "
+				"%llx, offset %x\n", SQUASHFS_INODE_BLK(inode),
+				offset, SQUASHFS_I(i)->start_block, next_block,
+				next_offset);
+		break;
+	}
+	case SQUASHFS_DIR_TYPE: {
+		struct squashfs_dir_inode *inodep = &id.dir;
+
+		if (!squashfs_read_metadata(s, inodep, block, offset,
+				sizeof(*inodep), &next_block, &next_offset))
+			goto failed_read;
+
+		i->i_nlink = le32_to_cpu(inodep->nlink);
+		i->i_size = le16_to_cpu(inodep->file_size);
+		i->i_op = &squashfs_dir_inode_ops;
+		i->i_fop = &squashfs_dir_ops;
+		i->i_mode |= S_IFDIR;
+		SQUASHFS_I(i)->start_block = le32_to_cpu(inodep->start_block);
+		SQUASHFS_I(i)->offset = le16_to_cpu(inodep->offset);
+		SQUASHFS_I(i)->dir_index_count = 0;
+		SQUASHFS_I(i)->parent_inode = le32_to_cpu(inodep->parent_inode);
+
+		TRACE("Directory inode %x:%x, start_block %llx, offset %x\n",
+				SQUASHFS_INODE_BLK(inode), offset,
+				SQUASHFS_I(i)->start_block,
+				le16_to_cpu(inodep->offset));
+		break;
+	}
+	case SQUASHFS_LDIR_TYPE: {
+		struct squashfs_ldir_inode *inodep = &id.ldir;
+
+		if (!squashfs_read_metadata(s, inodep, block, offset,
+				sizeof(*inodep), &next_block, &next_offset))
+			goto failed_read;
+
+		i->i_nlink = le32_to_cpu(inodep->nlink);
+		i->i_size = le32_to_cpu(inodep->file_size);
+		i->i_op = &squashfs_dir_inode_ops;
+		i->i_fop = &squashfs_dir_ops;
+		i->i_mode |= S_IFDIR;
+		SQUASHFS_I(i)->start_block = le32_to_cpu(inodep->start_block);
+		SQUASHFS_I(i)->offset = le16_to_cpu(inodep->offset);
+		SQUASHFS_I(i)->dir_index_start = next_block;
+		SQUASHFS_I(i)->dir_index_offset = next_offset;
+		SQUASHFS_I(i)->dir_index_count = le16_to_cpu(inodep->i_count);
+		SQUASHFS_I(i)->parent_inode = le32_to_cpu(inodep->parent_inode);
+
+		TRACE("Long directory inode %x:%x, start_block %llx, offset "
+				"%x\n", SQUASHFS_INODE_BLK(inode), offset,
+				SQUASHFS_I(i)->start_block,
+				le16_to_cpu(inodep->offset));
+		break;
+	}
+	case SQUASHFS_SYMLINK_TYPE: {
+		struct squashfs_symlink_inode *inodep = &id.symlink;
+
+		if (!squashfs_read_metadata(s, inodep, block, offset,
+				sizeof(*inodep), &next_block, &next_offset))
+			goto failed_read;
+
+		i->i_nlink = le32_to_cpu(inodep->nlink);
+		i->i_size = le32_to_cpu(inodep->symlink_size);
+		i->i_op = &page_symlink_inode_operations;
+		i->i_data.a_ops = &squashfs_symlink_aops;
+		i->i_mode |= S_IFLNK;
+		SQUASHFS_I(i)->start_block = next_block;
+		SQUASHFS_I(i)->offset = next_offset;
+
+		TRACE("Symbolic link inode %x:%x, start_block %llx, offset "
+				"%x\n", SQUASHFS_INODE_BLK(inode), offset,
+				next_block, next_offset);
+		break;
+	}
+	case SQUASHFS_BLKDEV_TYPE:
+	case SQUASHFS_CHRDEV_TYPE: {
+		struct squashfs_dev_inode *inodep = &id.dev;
+		unsigned int rdev;
+
+		if (!squashfs_read_metadata(s, inodep, block, offset,
+				sizeof(*inodep), &next_block, &next_offset))
+			goto failed_read;
+
+		i->i_nlink = le32_to_cpu(inodep->nlink);
+		i->i_mode |= (type == SQUASHFS_CHRDEV_TYPE) ? S_IFCHR : S_IFBLK;
+		rdev = le32_to_cpu(inodep->rdev);
+		init_special_inode(i, le16_to_cpu(i->i_mode),
+					new_decode_dev(rdev));
+
+		TRACE("Device inode %x:%x, rdev %x\n",
+				SQUASHFS_INODE_BLK(inode), offset, rdev);
+		break;
+	}
+	case SQUASHFS_FIFO_TYPE:
+	case SQUASHFS_SOCKET_TYPE: {
+		struct squashfs_ipc_inode *inodep = &id.ipc;
+
+		if (!squashfs_read_metadata(s, inodep, block, offset,
+				sizeof(*inodep), &next_block, &next_offset))
+			goto failed_read;
+
+		i->i_nlink = le32_to_cpu(inodep->nlink);
+		i->i_mode |= (type == SQUASHFS_FIFO_TYPE) ? S_IFIFO : S_IFSOCK;
+		init_special_inode(i, le16_to_cpu(i->i_mode), 0);
+		break;
+	}
+	default:
+		ERROR("Unknown inode type %d in squashfs_iget!\n", type);
+		goto failed_read1;
+	}
+
+	return 1;
+
+failed_read:
+	ERROR("Unable to read inode [%llx:%x]\n", block, offset);
+
+failed_read1:
+	make_bad_inode(i);
+	return 0;
+}
-- 
1.5.2.5


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-17 15:42 Subject: [PATCH 01/16] Squashfs: inode operations Phillip Lougher
@ 2008-10-17 16:53 ` Jörn Engel
  2008-10-21  0:32   ` Phillip Lougher
  2008-10-21 16:14   ` David P. Quigley
  2008-10-22 16:32 ` Geert Uytterhoeven
  2008-10-22 17:13 ` Geert Uytterhoeven
  2 siblings, 2 replies; 9+ messages in thread
From: Jörn Engel @ 2008-10-17 16:53 UTC (permalink / raw)
  To: Phillip Lougher
  Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird

None of the comments below are a reason against mainline inclusion, imo.
They should get handled, but whether that happens before or after a
merge doesn't really matter.

On Fri, 17 October 2008 16:42:50 +0100, Phillip Lougher wrote:
> 
> +#include <linux/squashfs_fs.h>
> +#include <linux/squashfs_fs_sb.h>
> +#include <linux/squashfs_fs_i.h>

Current verdict seems to be that these files should live in fs/squashfs/,
not include/linux/.  No kernel code beside squashfs needs the headers
and userspace tools should have a private copy.

> +static int squashfs_new_inode(struct super_block *s, struct inode *i,
> +				struct squashfs_base_inode *inodeb)
> +{
> +	if (squashfs_get_id(s, le16_to_cpu(inodeb->uid), &i->i_uid) == 0)
> +		goto out;
> +	if (squashfs_get_id(s, le16_to_cpu(inodeb->guid), &i->i_gid) == 0)
> +		goto out;
> +
> +	i->i_ino = le32_to_cpu(inodeb->inode_number);
> +	i->i_mtime.tv_sec = le32_to_cpu(inodeb->mtime);
> +	i->i_atime.tv_sec = i->i_mtime.tv_sec;
> +	i->i_ctime.tv_sec = i->i_mtime.tv_sec;
> +	i->i_mode = le16_to_cpu(inodeb->mode);
> +	i->i_size = 0;
> +
> +	return 1;
> +
> +out:
> +	return 0;
> +}

Most code uses "sb" and "inode", which I consider easier to read - if
only for consistency.

> +int squashfs_read_inode(struct inode *i, long long inode)

Is your "long long inode" what most filesystems call "inode->i_ino"?  It
seems to be.

> +	if (squashfs_new_inode(s, i, inodeb) == 0)
> +			goto failed_read;

Most linux functions return 0 on success and -ESOMETHING on error.  You
return 0 on error and 1 on success.  That makes it likely for someone
else to do something like

	err = squashfs_foo(bar);
	if (err)
		goto fail;

Oops.

Jörn

-- 
Measure. Don't tune for speed until you've measured, and even then
don't unless one part of the code overwhelms the rest.
-- Rob Pike

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-17 16:53 ` Jörn Engel
@ 2008-10-21  0:32   ` Phillip Lougher
  2008-10-21 16:14   ` David P. Quigley
  1 sibling, 0 replies; 9+ messages in thread
From: Phillip Lougher @ 2008-10-21  0:32 UTC (permalink / raw)
  To: Jörn Engel
  Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird

Jörn Engel wrote:
> None of the comments below are a reason against mainline inclusion, imo.
> They should get handled, but whether that happens before or after a
> merge doesn't really matter.

Yeah you're right regarding your comments.  That's where code-review 
comes in handy, to spot things you don't see because you're too used to 
the code.

I'm working on a re-spin incorporating your comments, and it should be 
ready tomorrow.

Thanks for the code-review.

Phillip

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-17 16:53 ` Jörn Engel
  2008-10-21  0:32   ` Phillip Lougher
@ 2008-10-21 16:14   ` David P. Quigley
  2008-10-21 18:03     ` Jörn Engel
  2008-10-22  8:48     ` Christoph Hellwig
  1 sibling, 2 replies; 9+ messages in thread
From: David P. Quigley @ 2008-10-21 16:14 UTC (permalink / raw)
  To: Jörn Engel
  Cc: Phillip Lougher, akpm, linux-embedded, linux-fsdevel,
	linux-kernel, tim.bird

On Fri, 2008-10-17 at 18:53 +0200, Jörn Engel wrote:
> None of the comments below are a reason against mainline inclusion, imo.
> They should get handled, but whether that happens before or after a
> merge doesn't really matter.
> 
> On Fri, 17 October 2008 16:42:50 +0100, Phillip Lougher wrote:
> > 
> > +#include <linux/squashfs_fs.h>
> > +#include <linux/squashfs_fs_sb.h>
> > +#include <linux/squashfs_fs_i.h>
> 
> Current verdict seems to be that these files should live in fs/squashfs/,
> not include/linux/.  No kernel code beside squashfs needs the headers
> and userspace tools should have a private copy.
> 
[Snip]

I looked at where filesystems such as ext3 store these and it seems to
be in include/linux. I'm assuming this is because usespace utilities
like fsck need them. It seems wrong for userspace tools to have their
own private copy since you can potentially have them out of sync with
the kernel you are running and it provides more chance for you
forgetting to update a structure somewhere. 

Dave


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-21 16:14   ` David P. Quigley
@ 2008-10-21 18:03     ` Jörn Engel
  2008-10-22  8:48     ` Christoph Hellwig
  1 sibling, 0 replies; 9+ messages in thread
From: Jörn Engel @ 2008-10-21 18:03 UTC (permalink / raw)
  To: David P. Quigley
  Cc: Phillip Lougher, akpm, linux-embedded, linux-fsdevel,
	linux-kernel, tim.bird

On Tue, 21 October 2008 12:14:26 -0400, David P. Quigley wrote:
> On Fri, 2008-10-17 at 18:53 +0200, Jörn Engel wrote:
> > None of the comments below are a reason against mainline inclusion, imo.
> > They should get handled, but whether that happens before or after a
> > merge doesn't really matter.
> > 
> > On Fri, 17 October 2008 16:42:50 +0100, Phillip Lougher wrote:
> > > 
> > > +#include <linux/squashfs_fs.h>
> > > +#include <linux/squashfs_fs_sb.h>
> > > +#include <linux/squashfs_fs_i.h>
> > 
> > Current verdict seems to be that these files should live in fs/squashfs/,
> > not include/linux/.  No kernel code beside squashfs needs the headers
> > and userspace tools should have a private copy.
> > 
> [Snip]
> 
> I looked at where filesystems such as ext3 store these and it seems to
> be in include/linux. I'm assuming this is because usespace utilities
> like fsck need them. It seems wrong for userspace tools to have their
> own private copy since you can potentially have them out of sync with
> the kernel you are running and it provides more chance for you
> forgetting to update a structure somewhere. 

Existing headers remain where they are.  New headers are supposed to
go... or at least that's what I was told to do.

And being out of sync is definitely not an argument you can use with a
filesystem.  The data on your disk doesn't magically change when you
upgrade a kernel.  Nor can you assume that any given filesystem is
accessed only by Linux.  If you change the format, then locating
external copies of the header will be the least of your problems.

Jörn

-- 
Do not stop an army on its way home.
-- Sun Tzu

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-21 16:14   ` David P. Quigley
  2008-10-21 18:03     ` Jörn Engel
@ 2008-10-22  8:48     ` Christoph Hellwig
  1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2008-10-22  8:48 UTC (permalink / raw)
  To: David P. Quigley
  Cc: J?rn Engel, Phillip Lougher, akpm, linux-embedded, linux-fsdevel,
	linux-kernel, tim.bird

On Tue, Oct 21, 2008 at 12:14:26PM -0400, David P. Quigley wrote:
> I looked at where filesystems such as ext3 store these and it seems to
> be in include/linux. I'm assuming this is because usespace utilities
> like fsck need them. It seems wrong for userspace tools to have their
> own private copy since you can potentially have them out of sync with
> the kernel you are running and it provides more chance for you
> forgetting to update a structure somewhere. 

All modern filesystems have it in their directories, and ext3 will have
that soon too.  The only thing that should go into include/linux are
defintions for ioctls if you have them.  It is absolutely intention that
the tools can get out of sync with the kernel, because that actually
keeps them compiling when you update things in the kernel - note that
a single on disk format can be represented by lots of different things
in C, and for various reaosons those can change once in a while in the
kernel.  It also allows you to actually compile the tools on a system
that doesn't have new enough kernel headers yet (e.g. debian
autobuilders) or even run the tools on various platforms that have no
or different kernel implementations.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-17 15:42 Subject: [PATCH 01/16] Squashfs: inode operations Phillip Lougher
  2008-10-17 16:53 ` Jörn Engel
@ 2008-10-22 16:32 ` Geert Uytterhoeven
  2008-10-22 17:13 ` Geert Uytterhoeven
  2 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2008-10-22 16:32 UTC (permalink / raw)
  To: Phillip Lougher
  Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird

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

On Fri, 17 Oct 2008, Phillip Lougher wrote:
> --- /dev/null
> +++ b/fs/squashfs/inode.c

> +static int squashfs_new_inode(struct super_block *s, struct inode *i,
> +				struct squashfs_base_inode *inodeb)
> +{
> +	if (squashfs_get_id(s, le16_to_cpu(inodeb->uid), &i->i_uid) == 0)
> +		goto out;
                ^^^^^^^^
> +	if (squashfs_get_id(s, le16_to_cpu(inodeb->guid), &i->i_gid) == 0)
> +		goto out;
                ^^^^^^^^

> +	return 1;
> +
> +out:
> +	return 0;
> +}

As there's nothing to clean up, you can just use `return 0' instead of `goto
out'.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-17 15:42 Subject: [PATCH 01/16] Squashfs: inode operations Phillip Lougher
  2008-10-17 16:53 ` Jörn Engel
  2008-10-22 16:32 ` Geert Uytterhoeven
@ 2008-10-22 17:13 ` Geert Uytterhoeven
  2008-10-23  8:42   ` Phillip Lougher
  2 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2008-10-22 17:13 UTC (permalink / raw)
  To: Phillip Lougher
  Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird

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

On Fri, 17 Oct 2008, Phillip Lougher wrote:
> --- /dev/null
> +++ b/fs/squashfs/inode.c

> +	case SQUASHFS_BLKDEV_TYPE:
> +	case SQUASHFS_CHRDEV_TYPE: {
> +		struct squashfs_dev_inode *inodep = &id.dev;
> +		unsigned int rdev;
> +
> +		if (!squashfs_read_metadata(s, inodep, block, offset,
> +				sizeof(*inodep), &next_block, &next_offset))
> +			goto failed_read;
> +
> +		i->i_nlink = le32_to_cpu(inodep->nlink);
> +		i->i_mode |= (type == SQUASHFS_CHRDEV_TYPE) ? S_IFCHR : S_IFBLK;
> +		rdev = le32_to_cpu(inodep->rdev);
> +		init_special_inode(i, le16_to_cpu(i->i_mode),
                                      ^^^^^^^^^^^
> +					new_decode_dev(rdev));
> +
> +		TRACE("Device inode %x:%x, rdev %x\n",
> +				SQUASHFS_INODE_BLK(inode), offset, rdev);
> +		break;
> +	}
> +	case SQUASHFS_FIFO_TYPE:
> +	case SQUASHFS_SOCKET_TYPE: {
> +		struct squashfs_ipc_inode *inodep = &id.ipc;
> +
> +		if (!squashfs_read_metadata(s, inodep, block, offset,
> +				sizeof(*inodep), &next_block, &next_offset))
> +			goto failed_read;
> +
> +		i->i_nlink = le32_to_cpu(inodep->nlink);
> +		i->i_mode |= (type == SQUASHFS_FIFO_TYPE) ? S_IFIFO : S_IFSOCK;
> +		init_special_inode(i, le16_to_cpu(i->i_mode), 0);
                                      ^^^^^^^^^^^
> +		break;

Sparse with endian checking (make C=2 CHECKFLAGS="-D__CHECK_ENDIAN__") complains
about these:

| fs/squashfs/inode.c:306:25: warning: cast to restricted __le16
| fs/squashfs/inode.c:324:25: warning: cast to restricted __le16

and it seems to be right, as inode.i_mode is not __le16. I think the le16_to_cpu()
should be removed.

BTW, there are also a few sparse warnings about different signednesses, so you
probably want to run sparse yourself, too.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Subject: [PATCH 01/16] Squashfs: inode operations
  2008-10-22 17:13 ` Geert Uytterhoeven
@ 2008-10-23  8:42   ` Phillip Lougher
  0 siblings, 0 replies; 9+ messages in thread
From: Phillip Lougher @ 2008-10-23  8:42 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: akpm, linux-embedded, linux-fsdevel, linux-kernel, tim.bird

Geert Uytterhoeven wrote:

> Sparse with endian checking (make C=2 CHECKFLAGS="-D__CHECK_ENDIAN__") complains
> aibout these:
> 
> | fs/squashfs/inode.c:306:25: warning: cast to restricted __le16
> | fs/squashfs/inode.c:324:25: warning: cast to restricted __le16
> 
> and it seems to be right, as inode.i_mode is not __le16. I think the le16_to_cpu()
> should be removed.

Yes, you're right.  Fixed thanks.

> 
> BTW, there are also a few sparse warnings about different signednesses, so you
> probably want to run sparse yourself, too.

I'll do that.

Phillip

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-10-23  8:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-17 15:42 Subject: [PATCH 01/16] Squashfs: inode operations Phillip Lougher
2008-10-17 16:53 ` Jörn Engel
2008-10-21  0:32   ` Phillip Lougher
2008-10-21 16:14   ` David P. Quigley
2008-10-21 18:03     ` Jörn Engel
2008-10-22  8:48     ` Christoph Hellwig
2008-10-22 16:32 ` Geert Uytterhoeven
2008-10-22 17:13 ` Geert Uytterhoeven
2008-10-23  8:42   ` Phillip Lougher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).