Re: Subject: [PATCH 01/16] Squashfs: inode operations
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 -- To unsubscribe from this list: send the line "unsubscribe linux-embedded" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 01/16] Squashfs: inode operations
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: [EMAIL PROTECTED]
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
Re: Subject: [PATCH 01/16] Squashfs: inode operations
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: [EMAIL PROTECTED]
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
Re: Subject: [PATCH 01/16] Squashfs: inode operations
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. -- To unsubscribe from this list: send the line "unsubscribe linux-embedded" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 01/16] Squashfs: inode operations
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 > > > +#include > > > +#include > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-embedded" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 01/16] Squashfs: inode operations
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 > > +#include > > +#include > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-embedded" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 01/16] Squashfs: inode operations
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 -- To unsubscribe from this list: send the line "unsubscribe linux-embedded" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Subject: [PATCH 01/16] Squashfs: inode operations
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
> +#include
> +#include
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
--
To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
