[PATCH 1/2] Squashfs: Let the number of fragments cached configurable

[Qixuan Wu]

Currently, squashfs fragments' cache size is only determined by
config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE. Users have
no way to change the value when they get the binary kernel.
Now make it be configured during booting or inserting module.

Signed-off-by: Qixuan Wu 
Signed-off-by: Chen Jie 
---
 fs/squashfs/super.c | 43 ++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index cf01e15..82f9e46 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -38,6 +38,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "squashfs_fs.h"
 #include "squashfs_fs_sb.h"
@@ -49,6 +50,37 @@
 static struct file_system_type squashfs_fs_type;
 static const struct super_operations squashfs_super_ops;
 
+static unsigned int squashfs_frags_cache_size = SQUASHFS_CACHED_FRAGMENTS;
+
+#ifndefMODULE
+static int __init squashfs_frags_cachesize_setup(char *buf)
+{
+   unsigned int arg;
+
+   if (!buf || kstrtouint(buf, 10, ) < 0) {
+   pr_err("Bad squashfs fragments' cache size, let it be %d\n",
+   SQUASHFS_CACHED_FRAGMENTS);
+   return 0;
+   }
+
+   if (arg < 3 || arg > 99) {
+   pr_warn("Squashfs fragments' cache size should be in the range "
+   "of [3,99]. Let it be %d\n", SQUASHFS_CACHED_FRAGMENTS);
+   return 0;
+   }
+
+   squashfs_frags_cache_size = arg;
+
+   return 0;
+}
+early_param("squashfs.frags_cache_size", squashfs_frags_cachesize_setup);
+
+#else
+module_param(squashfs_frags_cache_size, uint, 0444);
+MODULE_PARM_DESC(squashfs_frags_cache_size,
+   "Squashfs fragments' cache size, it should be in the range of [3,99]");
+#endif
+
 static const struct squashfs_decompressor *supported_squashfs_filesystem(short
major, short minor, short id)
 {
@@ -276,8 +308,17 @@ static int squashfs_fill_super(struct super_block *sb, 
void *data, int silent)
if (fragments == 0)
goto check_directory_table;
 
+   if ((squashfs_frags_cache_size < 3)
+   || (squashfs_frags_cache_size > 99)) {
+   ERROR("Fragments' cache size should be in the range of[3,99]."
+   "Let it be %d\n", SQUASHFS_CACHED_FRAGMENTS);
+   squashfs_frags_cache_size = SQUASHFS_CACHED_FRAGMENTS;
+   }
+
+   TRACE("Fragments' cache size %d\n", squashfs_frags_cache_size);
+
msblk->fragment_cache = squashfs_cache_init("fragment",
-   SQUASHFS_CACHED_FRAGMENTS, msblk->block_size);
+   squashfs_frags_cache_size, msblk->block_size);
if (msblk->fragment_cache == NULL) {
err = -ENOMEM;
goto failed_mount;
-- 
2.7.4

[PATCH 1/2] Squashfs: Let the number of fragments cached configurable

[Qixuan Wu]

Currently, squashfs fragments' cache size is only determined by
config option CONFIG_SQUASHFS_FRAGMENT_CACHE_SIZE. Users have
no way to change the value when they get the binary kernel.
Now make it be configured during booting or inserting module.

Signed-off-by: Qixuan Wu 
Signed-off-by: Chen Jie 
---
 fs/squashfs/super.c | 43 ++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index cf01e15..82f9e46 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -38,6 +38,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "squashfs_fs.h"
 #include "squashfs_fs_sb.h"
@@ -49,6 +50,37 @@
 static struct file_system_type squashfs_fs_type;
 static const struct super_operations squashfs_super_ops;
 
+static unsigned int squashfs_frags_cache_size = SQUASHFS_CACHED_FRAGMENTS;
+
+#ifndefMODULE
+static int __init squashfs_frags_cachesize_setup(char *buf)
+{
+   unsigned int arg;
+
+   if (!buf || kstrtouint(buf, 10, ) < 0) {
+   pr_err("Bad squashfs fragments' cache size, let it be %d\n",
+   SQUASHFS_CACHED_FRAGMENTS);
+   return 0;
+   }
+
+   if (arg < 3 || arg > 99) {
+   pr_warn("Squashfs fragments' cache size should be in the range "
+   "of [3,99]. Let it be %d\n", SQUASHFS_CACHED_FRAGMENTS);
+   return 0;
+   }
+
+   squashfs_frags_cache_size = arg;
+
+   return 0;
+}
+early_param("squashfs.frags_cache_size", squashfs_frags_cachesize_setup);
+
+#else
+module_param(squashfs_frags_cache_size, uint, 0444);
+MODULE_PARM_DESC(squashfs_frags_cache_size,
+   "Squashfs fragments' cache size, it should be in the range of [3,99]");
+#endif
+
 static const struct squashfs_decompressor *supported_squashfs_filesystem(short
major, short minor, short id)
 {
@@ -276,8 +308,17 @@ static int squashfs_fill_super(struct super_block *sb, 
void *data, int silent)
if (fragments == 0)
goto check_directory_table;
 
+   if ((squashfs_frags_cache_size < 3)
+   || (squashfs_frags_cache_size > 99)) {
+   ERROR("Fragments' cache size should be in the range of[3,99]."
+   "Let it be %d\n", SQUASHFS_CACHED_FRAGMENTS);
+   squashfs_frags_cache_size = SQUASHFS_CACHED_FRAGMENTS;
+   }
+
+   TRACE("Fragments' cache size %d\n", squashfs_frags_cache_size);
+
msblk->fragment_cache = squashfs_cache_init("fragment",
-   SQUASHFS_CACHED_FRAGMENTS, msblk->block_size);
+   squashfs_frags_cache_size, msblk->block_size);
if (msblk->fragment_cache == NULL) {
err = -ENOMEM;
goto failed_mount;
-- 
2.7.4

[PATCH 1/2] Squashfs: add LZ4 compression support

[Phillip Lougher]

Add support for reading file systems compressed with the
LZ4 compression algorithm.

This patch adds the LZ4 decompressor wrapper code.

Signed-off-by: Phillip Lougher 
---
 fs/squashfs/lz4_wrapper.c |  142 +
 fs/squashfs/squashfs_fs.h |1 +
 2 files changed, 143 insertions(+)
 create mode 100644 fs/squashfs/lz4_wrapper.c

diff --git a/fs/squashfs/lz4_wrapper.c b/fs/squashfs/lz4_wrapper.c
new file mode 100644
index 000..c31e2bc
--- /dev/null
+++ b/fs/squashfs/lz4_wrapper.c
@@ -0,0 +1,142 @@
+/*
+ * Copyright (c) 2013, 2014
+ * Phillip Lougher 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "squashfs_fs.h"
+#include "squashfs_fs_sb.h"
+#include "squashfs.h"
+#include "decompressor.h"
+#include "page_actor.h"
+
+#define LZ4_LEGACY 1
+
+struct lz4_comp_opts {
+   __le32 version;
+   __le32 flags;
+};
+
+struct squashfs_lz4 {
+   void *input;
+   void *output;
+};
+
+
+static void *lz4_comp_opts(struct squashfs_sb_info *msblk,
+   void *buff, int len)
+{
+   struct lz4_comp_opts *comp_opts = buff;
+
+   /* LZ4 compressed filesystems always have compression options */
+   if (comp_opts == NULL || len < sizeof(*comp_opts))
+   return ERR_PTR(-EIO);
+
+   if (le32_to_cpu(comp_opts->version) != LZ4_LEGACY) {
+   /* LZ4 format currently used by the kernel is the 'legacy'
+* format */
+   ERROR("Unknown LZ4 version\n");
+   return ERR_PTR(-EINVAL);
+   }
+
+   return NULL;
+}
+
+
+static void *lz4_init(struct squashfs_sb_info *msblk, void *buff)
+{
+   int block_size = max_t(int, msblk->block_size, SQUASHFS_METADATA_SIZE);
+   struct squashfs_lz4 *stream;
+
+   stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+   if (stream == NULL)
+   goto failed;
+   stream->input = vmalloc(block_size);
+   if (stream->input == NULL)
+   goto failed2;
+   stream->output = vmalloc(block_size);
+   if (stream->output == NULL)
+   goto failed3;
+
+   return stream;
+
+failed3:
+   vfree(stream->input);
+failed2:
+   kfree(stream);
+failed:
+   ERROR("Failed to initialise LZ4 decompressor\n");
+   return ERR_PTR(-ENOMEM);
+}
+
+
+static void lz4_free(void *strm)
+{
+   struct squashfs_lz4 *stream = strm;
+
+   if (stream) {
+   vfree(stream->input);
+   vfree(stream->output);
+   }
+   kfree(stream);
+}
+
+
+static int lz4_uncompress(struct squashfs_sb_info *msblk, void *strm,
+   struct buffer_head **bh, int b, int offset, int length,
+   struct squashfs_page_actor *output)
+{
+   struct squashfs_lz4 *stream = strm;
+   void *buff = stream->input, *data;
+   int avail, i, bytes = length, res;
+   size_t dest_len = output->length;
+
+   for (i = 0; i < b; i++) {
+   avail = min(bytes, msblk->devblksize - offset);
+   memcpy(buff, bh[i]->b_data + offset, avail);
+   buff += avail;
+   bytes -= avail;
+   offset = 0;
+   put_bh(bh[i]);
+   }
+
+   res = lz4_decompress_unknownoutputsize(stream->input, length,
+   stream->output, _len);
+   if (res)
+   return -EIO;
+
+   bytes = dest_len;
+   data = squashfs_first_page(output);
+   buff = stream->output;
+   while (data) {
+   if (bytes <= PAGE_CACHE_SIZE) {
+   memcpy(data, buff, bytes);
+   break;
+   }
+   memcpy(data, buff, PAGE_CACHE_SIZE);
+   buff += PAGE_CACHE_SIZE;
+   bytes -= PAGE_CACHE_SIZE;
+   data = squashfs_next_page(output);
+   }
+   squashfs_finish_page(output);
+
+   return dest_len;
+}
+
+const struct squashfs_decompressor squashfs_lz4_comp_ops = {
+   .init = lz4_init,
+   .comp_opts = lz4_comp_opts,
+   .free = lz4_free,
+   .decompress = lz4_uncompress,
+   .id = LZ4_COMPRESSION,
+   .name = "lz4",
+   .supported = 1
+};
diff --git a/fs/squashfs/squashfs_fs.h b/fs/squashfs/squashfs_fs.h
index 4b2beda..506f4ba 100644
--- a/fs/squashfs/squashfs_fs.h
+++ b/fs/squashfs/squashfs_fs.h
@@ -240,6 +240,7 @@ struct meta_index {
 #define LZMA_COMPRESSION   2
 #define LZO_COMPRESSION3
 #define XZ_COMPRESSION 4
+#define LZ4_COMPRESSION5
 
 struct squashfs_super_block {
__le32  s_magic;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] Squashfs: add LZ4 compression support

[Phillip Lougher]

Add support for reading file systems compressed with the
LZ4 compression algorithm.

This patch adds the LZ4 decompressor wrapper code.

Signed-off-by: Phillip Lougher phil...@squashfs.org.uk
---
 fs/squashfs/lz4_wrapper.c |  142 +
 fs/squashfs/squashfs_fs.h |1 +
 2 files changed, 143 insertions(+)
 create mode 100644 fs/squashfs/lz4_wrapper.c

diff --git a/fs/squashfs/lz4_wrapper.c b/fs/squashfs/lz4_wrapper.c
new file mode 100644
index 000..c31e2bc
--- /dev/null
+++ b/fs/squashfs/lz4_wrapper.c
@@ -0,0 +1,142 @@
+/*
+ * Copyright (c) 2013, 2014
+ * Phillip Lougher phil...@squashfs.org.uk
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include linux/buffer_head.h
+#include linux/mutex.h
+#include linux/slab.h
+#include linux/vmalloc.h
+#include linux/lz4.h
+
+#include squashfs_fs.h
+#include squashfs_fs_sb.h
+#include squashfs.h
+#include decompressor.h
+#include page_actor.h
+
+#define LZ4_LEGACY 1
+
+struct lz4_comp_opts {
+   __le32 version;
+   __le32 flags;
+};
+
+struct squashfs_lz4 {
+   void *input;
+   void *output;
+};
+
+
+static void *lz4_comp_opts(struct squashfs_sb_info *msblk,
+   void *buff, int len)
+{
+   struct lz4_comp_opts *comp_opts = buff;
+
+   /* LZ4 compressed filesystems always have compression options */
+   if (comp_opts == NULL || len  sizeof(*comp_opts))
+   return ERR_PTR(-EIO);
+
+   if (le32_to_cpu(comp_opts-version) != LZ4_LEGACY) {
+   /* LZ4 format currently used by the kernel is the 'legacy'
+* format */
+   ERROR(Unknown LZ4 version\n);
+   return ERR_PTR(-EINVAL);
+   }
+
+   return NULL;
+}
+
+
+static void *lz4_init(struct squashfs_sb_info *msblk, void *buff)
+{
+   int block_size = max_t(int, msblk-block_size, SQUASHFS_METADATA_SIZE);
+   struct squashfs_lz4 *stream;
+
+   stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+   if (stream == NULL)
+   goto failed;
+   stream-input = vmalloc(block_size);
+   if (stream-input == NULL)
+   goto failed2;
+   stream-output = vmalloc(block_size);
+   if (stream-output == NULL)
+   goto failed3;
+
+   return stream;
+
+failed3:
+   vfree(stream-input);
+failed2:
+   kfree(stream);
+failed:
+   ERROR(Failed to initialise LZ4 decompressor\n);
+   return ERR_PTR(-ENOMEM);
+}
+
+
+static void lz4_free(void *strm)
+{
+   struct squashfs_lz4 *stream = strm;
+
+   if (stream) {
+   vfree(stream-input);
+   vfree(stream-output);
+   }
+   kfree(stream);
+}
+
+
+static int lz4_uncompress(struct squashfs_sb_info *msblk, void *strm,
+   struct buffer_head **bh, int b, int offset, int length,
+   struct squashfs_page_actor *output)
+{
+   struct squashfs_lz4 *stream = strm;
+   void *buff = stream-input, *data;
+   int avail, i, bytes = length, res;
+   size_t dest_len = output-length;
+
+   for (i = 0; i  b; i++) {
+   avail = min(bytes, msblk-devblksize - offset);
+   memcpy(buff, bh[i]-b_data + offset, avail);
+   buff += avail;
+   bytes -= avail;
+   offset = 0;
+   put_bh(bh[i]);
+   }
+
+   res = lz4_decompress_unknownoutputsize(stream-input, length,
+   stream-output, dest_len);
+   if (res)
+   return -EIO;
+
+   bytes = dest_len;
+   data = squashfs_first_page(output);
+   buff = stream-output;
+   while (data) {
+   if (bytes = PAGE_CACHE_SIZE) {
+   memcpy(data, buff, bytes);
+   break;
+   }
+   memcpy(data, buff, PAGE_CACHE_SIZE);
+   buff += PAGE_CACHE_SIZE;
+   bytes -= PAGE_CACHE_SIZE;
+   data = squashfs_next_page(output);
+   }
+   squashfs_finish_page(output);
+
+   return dest_len;
+}
+
+const struct squashfs_decompressor squashfs_lz4_comp_ops = {
+   .init = lz4_init,
+   .comp_opts = lz4_comp_opts,
+   .free = lz4_free,
+   .decompress = lz4_uncompress,
+   .id = LZ4_COMPRESSION,
+   .name = lz4,
+   .supported = 1
+};
diff --git a/fs/squashfs/squashfs_fs.h b/fs/squashfs/squashfs_fs.h
index 4b2beda..506f4ba 100644
--- a/fs/squashfs/squashfs_fs.h
+++ b/fs/squashfs/squashfs_fs.h
@@ -240,6 +240,7 @@ struct meta_index {
 #define LZMA_COMPRESSION   2
 #define LZO_COMPRESSION3
 #define XZ_COMPRESSION 4
+#define LZ4_COMPRESSION5
 
 struct squashfs_super_block {
__le32  s_magic;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

Re: [PATCH 1/2] Squashfs: add LZ4 compression support

[Joe Perches]

On Mon, 2013-07-22 at 03:21 +0100, Phillip Lougher wrote:
> Add support for reading file systems compressed with the
> LZ4 compression algorithm.

Some whitespace trivia and a naming comment.

> diff --git a/fs/squashfs/lz4_wrapper.c b/fs/squashfs/lz4_wrapper.c
[]
> +static void *lz4_init(struct squashfs_sb_info *msblk, void *buff, int len)
[]
> + /* LZ4 compressed filesystems always have compression options */
> + if(comp_opts == NULL || len < sizeof(*comp_opts)) {

space after ifs please.

> + if(le32_to_cpu(comp_opts->version) != LZ4_LEGACY) {

> +
> +
> +static void lz4_free(void *strm)

Single blank line between functions

> +static int lz4_uncompress(struct squashfs_sb_info *msblk, void **buffer,
> + struct buffer_head **bh, int b, int offset, int length, int srclength,
> + int pages)
> +{
> + struct squashfs_lz4 *stream = msblk->stream;
> + void *buff = stream->input;

It's not particularly nice to have both buffer and buff
in the same function.

Maybe void *input though char *input would be better.

> + int avail, i, bytes = length, res;
> + size_t dest_len = srclength;
> +
> + mutex_lock(>read_data_mutex);
> +
> + for (i = 0; i < b; i++) {
> + wait_on_buffer(bh[i]);
> + if (!buffer_uptodate(bh[i]))
> + goto block_release;
> +
> + avail = min(bytes, msblk->devblksize - offset);
> + memcpy(buff, bh[i]->b_data + offset, avail);
> + buff += avail;
> + bytes -= avail;
> + offset = 0;
> + put_bh(bh[i]);
> + }


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] Squashfs: add LZ4 compression support

[Phillip Lougher]

Add support for reading file systems compressed with the
LZ4 compression algorithm.

This patch adds the LZ4 decompressor wrapper code.

Signed-off-by: Phillip Lougher 
---
 fs/squashfs/lz4_wrapper.c |  163 +
 fs/squashfs/squashfs_fs.h |1 +
 2 files changed, 164 insertions(+)
 create mode 100644 fs/squashfs/lz4_wrapper.c

diff --git a/fs/squashfs/lz4_wrapper.c b/fs/squashfs/lz4_wrapper.c
new file mode 100644
index 000..681ed94
--- /dev/null
+++ b/fs/squashfs/lz4_wrapper.c
@@ -0,0 +1,163 @@
+/*
+ * Squashfs - a compressed read only filesystem for Linux
+ *
+ * Copyright (c) 2013
+ * Phillip Lougher 
+ *
+ * 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.
+ *
+ * lz4_wrapper.c
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "squashfs_fs.h"
+#include "squashfs_fs_sb.h"
+#include "squashfs.h"
+#include "decompressor.h"
+
+#define LZ4_LEGACY 1
+
+struct lz4_comp_opts {
+   __le32 version;
+   __le32 flags;
+};
+
+struct squashfs_lz4 {
+   void *input;
+   void *output;
+};
+
+
+
+static void *lz4_init(struct squashfs_sb_info *msblk, void *buff, int len)
+{
+   struct lz4_comp_opts *comp_opts = buff;
+   int block_size = max_t(int, msblk->block_size, SQUASHFS_METADATA_SIZE);
+   struct squashfs_lz4 *stream;
+   int err = -ENOMEM;
+
+   /* LZ4 compressed filesystems always have compression options */
+   if(comp_opts == NULL || len < sizeof(*comp_opts)) {
+   err = -EIO;
+   goto failed;
+   }
+   if(le32_to_cpu(comp_opts->version) != LZ4_LEGACY) {
+   /* LZ4 format currently used by the kernel is the 'legacy'
+* format */
+   ERROR("Unknown LZ4 version\n");
+   err = -EINVAL;
+   goto failed;
+   }
+
+   stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+   if (stream == NULL)
+   goto failed;
+   stream->input = vmalloc(block_size);
+   if (stream->input == NULL)
+   goto failed2;
+   stream->output = vmalloc(block_size);
+   if (stream->output == NULL)
+   goto failed3;
+
+   return stream;
+
+failed3:
+   vfree(stream->input);
+failed2:
+   kfree(stream);
+failed:
+   ERROR("Failed to initialise LZ4 decompressor\n");
+   return ERR_PTR(err);
+}
+
+
+static void lz4_free(void *strm)
+{
+   struct squashfs_lz4 *stream = strm;
+
+   if (stream) {
+   vfree(stream->input);
+   vfree(stream->output);
+   }
+   kfree(stream);
+}
+
+
+static int lz4_uncompress(struct squashfs_sb_info *msblk, void **buffer,
+   struct buffer_head **bh, int b, int offset, int length, int srclength,
+   int pages)
+{
+   struct squashfs_lz4 *stream = msblk->stream;
+   void *buff = stream->input;
+   int avail, i, bytes = length, res;
+   size_t dest_len = srclength;
+
+   mutex_lock(>read_data_mutex);
+
+   for (i = 0; i < b; i++) {
+   wait_on_buffer(bh[i]);
+   if (!buffer_uptodate(bh[i]))
+   goto block_release;
+
+   avail = min(bytes, msblk->devblksize - offset);
+   memcpy(buff, bh[i]->b_data + offset, avail);
+   buff += avail;
+   bytes -= avail;
+   offset = 0;
+   put_bh(bh[i]);
+   }
+
+   res = lz4_decompress_unknownoutputsize(stream->input, length,
+   stream->output, _len);
+   if (res)
+   goto failed;
+
+   bytes = dest_len;
+   for (i = 0, buff = stream->output; bytes && i < pages; i++) {
+   avail = min_t(int, bytes, PAGE_CACHE_SIZE);
+   memcpy(buffer[i], buff, avail);
+   buff += avail;
+   bytes -= avail;
+   }
+   if (bytes)
+   goto failed;
+
+   mutex_unlock(>read_data_mutex);
+   return dest_len;
+
+block_release:
+   for (; i < b; i++)
+   put_bh(bh[i]);
+
+failed:
+   mutex_unlock(>read_data_mutex);
+
+   ERROR("lz4 decompression failed, data probably corrupt\n");
+   return -EIO;
+}
+
+const struct squashfs_decompressor squashfs_lz4_comp_ops = {
+   .init = lz4_init,
+   .free = lz4_free,
+   

[PATCH 1/2] Squashfs: add LZ4 compression support

[Phillip Lougher]

Add support for reading file systems compressed with the
LZ4 compression algorithm.

This patch adds the LZ4 decompressor wrapper code.

Signed-off-by: Phillip Lougher phil...@squashfs.org.uk
---
 fs/squashfs/lz4_wrapper.c |  163 +
 fs/squashfs/squashfs_fs.h |1 +
 2 files changed, 164 insertions(+)
 create mode 100644 fs/squashfs/lz4_wrapper.c

diff --git a/fs/squashfs/lz4_wrapper.c b/fs/squashfs/lz4_wrapper.c
new file mode 100644
index 000..681ed94
--- /dev/null
+++ b/fs/squashfs/lz4_wrapper.c
@@ -0,0 +1,163 @@
+/*
+ * Squashfs - a compressed read only filesystem for Linux
+ *
+ * Copyright (c) 2013
+ * Phillip Lougher phil...@squashfs.org.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.
+ *
+ * lz4_wrapper.c
+ */
+
+#include linux/buffer_head.h
+#include linux/mutex.h
+#include linux/slab.h
+#include linux/vmalloc.h
+#include linux/lz4.h
+
+#include squashfs_fs.h
+#include squashfs_fs_sb.h
+#include squashfs.h
+#include decompressor.h
+
+#define LZ4_LEGACY 1
+
+struct lz4_comp_opts {
+   __le32 version;
+   __le32 flags;
+};
+
+struct squashfs_lz4 {
+   void *input;
+   void *output;
+};
+
+
+
+static void *lz4_init(struct squashfs_sb_info *msblk, void *buff, int len)
+{
+   struct lz4_comp_opts *comp_opts = buff;
+   int block_size = max_t(int, msblk-block_size, SQUASHFS_METADATA_SIZE);
+   struct squashfs_lz4 *stream;
+   int err = -ENOMEM;
+
+   /* LZ4 compressed filesystems always have compression options */
+   if(comp_opts == NULL || len  sizeof(*comp_opts)) {
+   err = -EIO;
+   goto failed;
+   }
+   if(le32_to_cpu(comp_opts-version) != LZ4_LEGACY) {
+   /* LZ4 format currently used by the kernel is the 'legacy'
+* format */
+   ERROR(Unknown LZ4 version\n);
+   err = -EINVAL;
+   goto failed;
+   }
+
+   stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+   if (stream == NULL)
+   goto failed;
+   stream-input = vmalloc(block_size);
+   if (stream-input == NULL)
+   goto failed2;
+   stream-output = vmalloc(block_size);
+   if (stream-output == NULL)
+   goto failed3;
+
+   return stream;
+
+failed3:
+   vfree(stream-input);
+failed2:
+   kfree(stream);
+failed:
+   ERROR(Failed to initialise LZ4 decompressor\n);
+   return ERR_PTR(err);
+}
+
+
+static void lz4_free(void *strm)
+{
+   struct squashfs_lz4 *stream = strm;
+
+   if (stream) {
+   vfree(stream-input);
+   vfree(stream-output);
+   }
+   kfree(stream);
+}
+
+
+static int lz4_uncompress(struct squashfs_sb_info *msblk, void **buffer,
+   struct buffer_head **bh, int b, int offset, int length, int srclength,
+   int pages)
+{
+   struct squashfs_lz4 *stream = msblk-stream;
+   void *buff = stream-input;
+   int avail, i, bytes = length, res;
+   size_t dest_len = srclength;
+
+   mutex_lock(msblk-read_data_mutex);
+
+   for (i = 0; i  b; i++) {
+   wait_on_buffer(bh[i]);
+   if (!buffer_uptodate(bh[i]))
+   goto block_release;
+
+   avail = min(bytes, msblk-devblksize - offset);
+   memcpy(buff, bh[i]-b_data + offset, avail);
+   buff += avail;
+   bytes -= avail;
+   offset = 0;
+   put_bh(bh[i]);
+   }
+
+   res = lz4_decompress_unknownoutputsize(stream-input, length,
+   stream-output, dest_len);
+   if (res)
+   goto failed;
+
+   bytes = dest_len;
+   for (i = 0, buff = stream-output; bytes  i  pages; i++) {
+   avail = min_t(int, bytes, PAGE_CACHE_SIZE);
+   memcpy(buffer[i], buff, avail);
+   buff += avail;
+   bytes -= avail;
+   }
+   if (bytes)
+   goto failed;
+
+   mutex_unlock(msblk-read_data_mutex);
+   return dest_len;
+
+block_release:
+   for (; i  b; i++)
+   put_bh(bh[i]);
+
+failed:
+   mutex_unlock(msblk-read_data_mutex);
+
+   ERROR(lz4 decompression failed, data probably corrupt\n);
+   return -EIO;
+}
+
+const struct 

Re: [PATCH 1/2] Squashfs: add LZ4 compression support

[Joe Perches]

On Mon, 2013-07-22 at 03:21 +0100, Phillip Lougher wrote:
 Add support for reading file systems compressed with the
 LZ4 compression algorithm.

Some whitespace trivia and a naming comment.

 diff --git a/fs/squashfs/lz4_wrapper.c b/fs/squashfs/lz4_wrapper.c
[]
 +static void *lz4_init(struct squashfs_sb_info *msblk, void *buff, int len)
[]
 + /* LZ4 compressed filesystems always have compression options */
 + if(comp_opts == NULL || len  sizeof(*comp_opts)) {

space after ifs please.

 + if(le32_to_cpu(comp_opts-version) != LZ4_LEGACY) {

 +
 +
 +static void lz4_free(void *strm)

Single blank line between functions

 +static int lz4_uncompress(struct squashfs_sb_info *msblk, void **buffer,
 + struct buffer_head **bh, int b, int offset, int length, int srclength,
 + int pages)
 +{
 + struct squashfs_lz4 *stream = msblk-stream;
 + void *buff = stream-input;

It's not particularly nice to have both buffer and buff
in the same function.

Maybe void *input though char *input would be better.

 + int avail, i, bytes = length, res;
 + size_t dest_len = srclength;
 +
 + mutex_lock(msblk-read_data_mutex);
 +
 + for (i = 0; i  b; i++) {
 + wait_on_buffer(bh[i]);
 + if (!buffer_uptodate(bh[i]))
 + goto block_release;
 +
 + avail = min(bytes, msblk-devblksize - offset);
 + memcpy(buff, bh[i]-b_data + offset, avail);
 + buff += avail;
 + bytes -= avail;
 + offset = 0;
 + put_bh(bh[i]);
 + }


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Function stack size usage (was [PATCH][1/2] SquashFS)

[Andrew Morton]

Phillip Lougher <[EMAIL PROTECTED]> wrote:
>
> Andrew Morton wrote:
> > Phillip Lougher <[EMAIL PROTECTED]> wrote:
> >>
> >>+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 = >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.
> > 
> 
> A lot of the functions use a fair amount of stack (I never thought it 
> was excessive)...  This is the result of running checkstack.pl against 
> the code on Intel.
> 
> 0x3a3c get_dir_index_using_name:596
> 0x0d80 squashfs_iget:   488
> 0x44d8 squashfs_lookup: 380
> 0x3d00 squashfs_readdir:372
> 0x20fe squashfs_fill_super: 316
> 0x31b8 squashfs_readpage:   308
> 0x2f5c read_blocklist:  296
> 0x3634 squashfs_readpage4K: 284
> 
> A couple of these functions show a fair amount of stack use.  What is 
> the maximum acceptable usage,

There's no hard-and-fast rule.  The conditions running up to a stack
overrun are necessarily complex, and rare.  But you can see that for a
twenty or thirty function deep call stack, 500 bytes is a big bite out of
4k.

> i.e. do any of the above functions need 
> work to reduce their stack usage?

I'd say so, yes.  If at all possible.
-
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/

Function stack size usage (was [PATCH][1/2] SquashFS)

[Phillip Lougher]

Andrew Morton wrote:
Phillip Lougher <[EMAIL PROTECTED]> wrote:
+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 = >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.
A lot of the functions use a fair amount of stack (I never thought it 
was excessive)...  This is the result of running checkstack.pl against 
the code on Intel.

0x3a3c get_dir_index_using_name:596
0x0d80 squashfs_iget:   488
0x44d8 squashfs_lookup: 380
0x3d00 squashfs_readdir:372
0x20fe squashfs_fill_super: 316
0x31b8 squashfs_readpage:   308
0x2f5c read_blocklist:  296
0x3634 squashfs_readpage4K: 284
A couple of these functions show a fair amount of stack use.  What is 
the maximum acceptable usage, i.e. do any of the above functions need 
work to reduce their stack usage?

Thanks
Phillip
-
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/

Function stack size usage (was [PATCH][1/2] SquashFS)

[Phillip Lougher]

Andrew Morton wrote:
Phillip Lougher [EMAIL PROTECTED] wrote:
+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.
A lot of the functions use a fair amount of stack (I never thought it 
was excessive)...  This is the result of running checkstack.pl against 
the code on Intel.

0x3a3c get_dir_index_using_name:596
0x0d80 squashfs_iget:   488
0x44d8 squashfs_lookup: 380
0x3d00 squashfs_readdir:372
0x20fe squashfs_fill_super: 316
0x31b8 squashfs_readpage:   308
0x2f5c read_blocklist:  296
0x3634 squashfs_readpage4K: 284
A couple of these functions show a fair amount of stack use.  What is 
the maximum acceptable usage, i.e. do any of the above functions need 
work to reduce their stack usage?

Thanks
Phillip
-
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/

Re: Function stack size usage (was [PATCH][1/2] SquashFS)

[Andrew Morton]

Phillip Lougher [EMAIL PROTECTED] wrote:

 Andrew Morton wrote:
  Phillip Lougher [EMAIL PROTECTED] wrote:
 
 +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.
  
 
 A lot of the functions use a fair amount of stack (I never thought it 
 was excessive)...  This is the result of running checkstack.pl against 
 the code on Intel.
 
 0x3a3c get_dir_index_using_name:596
 0x0d80 squashfs_iget:   488
 0x44d8 squashfs_lookup: 380
 0x3d00 squashfs_readdir:372
 0x20fe squashfs_fill_super: 316
 0x31b8 squashfs_readpage:   308
 0x2f5c read_blocklist:  296
 0x3634 squashfs_readpage4K: 284
 
 A couple of these functions show a fair amount of stack use.  What is 
 the maximum acceptable usage,

There's no hard-and-fast rule.  The conditions running up to a stack
overrun are necessarily complex, and rare.  But you can see that for a
twenty or thirty function deep call stack, 500 bytes is a big bite out of
4k.

 i.e. do any of the above functions need 
 work to reduce their stack usage?

I'd say so, yes.  If at all possible.
-
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/

Re: [PATCH][1/2] SquashFS

[jerome lacoste]

On Tue, 15 Mar 2005 17:50:02 -0800, Junio C Hamano <[EMAIL PROTECTED]> wrote:
> > "PJ" == Paul Jackson <[EMAIL PROTECTED]> writes:
> 
> PJ> There is not a concensus (nor a King Penguin dictate) between the
> PJ> "while(1)" and "for(;;)" style to document.
> 
> FWIW, linux-0.01 has four uses of "while (1)" and two uses of
> "for (;;)" ;-).
> 
> ./fs/inode.c:   while (1) {
> ./fs/namei.c:   while (1) {
> ./fs/namei.c:   while (1) {
> ./kernel/sched.c:   while (1) {
> 
> ./init/main.c:  for(;;) pause();
> ./kernel/panic.c:   for(;;);
> 
> What is interesting here is that the King Penguin used these two
> constructs with consistency.  The "while (1)" form was used with
> normal exit routes with "if (...) break" inside; while the
> "for(;;)" form was used only in unusual "the thread of control
> should get stuck here forever" cases.
> 
> So, Phillip's decision to go back to his original while(1) style
> seems to be in line with the style used in the original Linux
> kernel ;-).

After the Pinguin janitors, now comes the Pinguin archeologists.

This starts to be lemmingesque :)

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

Re: [PATCH][1/2] SquashFS

[jerome lacoste]

On Tue, 15 Mar 2005 17:50:02 -0800, Junio C Hamano [EMAIL PROTECTED] wrote:
  PJ == Paul Jackson [EMAIL PROTECTED] writes:
 
 PJ There is not a concensus (nor a King Penguin dictate) between the
 PJ while(1) and for(;;) style to document.
 
 FWIW, linux-0.01 has four uses of while (1) and two uses of
 for (;;) ;-).
 
 ./fs/inode.c:   while (1) {
 ./fs/namei.c:   while (1) {
 ./fs/namei.c:   while (1) {
 ./kernel/sched.c:   while (1) {
 
 ./init/main.c:  for(;;) pause();
 ./kernel/panic.c:   for(;;);
 
 What is interesting here is that the King Penguin used these two
 constructs with consistency.  The while (1) form was used with
 normal exit routes with if (...) break inside; while the
 for(;;) form was used only in unusual the thread of control
 should get stuck here forever cases.
 
 So, Phillip's decision to go back to his original while(1) style
 seems to be in line with the style used in the original Linux
 kernel ;-).

After the Pinguin janitors, now comes the Pinguin archeologists.

This starts to be lemmingesque :)

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

Re: [PATCH][1/2] SquashFS

[Paul Jackson]

>  the King Penguin used these two constructs with consistency:

Nice distinction - thanks.

-- 
  I won't rest till it's the best ...
  Programmer, Linux Scalability
  Paul Jackson <[EMAIL PROTECTED]> 1.650.933.1373, 
1.925.600.0401
-
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/

Re: [PATCH][1/2] SquashFS

[Junio C Hamano]

> "PJ" == Paul Jackson <[EMAIL PROTECTED]> writes:

PJ> There is not a concensus (nor a King Penguin dictate) between the
PJ> "while(1)" and "for(;;)" style to document.

FWIW, linux-0.01 has four uses of "while (1)" and two uses of
"for (;;)" ;-).

./fs/inode.c:   while (1) {
./fs/namei.c:   while (1) {
./fs/namei.c:   while (1) {
./kernel/sched.c:   while (1) {

./init/main.c:  for(;;) pause();
./kernel/panic.c:   for(;;);

What is interesting here is that the King Penguin used these two
constructs with consistency.  The "while (1)" form was used with
normal exit routes with "if (...) break" inside; while the
"for(;;)" form was used only in unusual "the thread of control
should get stuck here forever" cases.

So, Phillip's decision to go back to his original while(1) style
seems to be in line with the style used in the original Linux
kernel ;-).

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

Re: [PATCH][1/2] SquashFS

[Paul Jackson]

> >>Shouldn't issues like this be in the coding style document?

There is not a concensus (nor a King Penguin dictate) between the
"while(1)" and "for(;;)" style to document.  If this were a
frequently asked question, I suppose someone would eventually
note in a coding style doc that either style is acceptable.


> It's a shame the 'rather trivial' issue got picked up in the first place 

Not a shame at all.  Such coding style issues are well known to be a
proper subject for discussion here.

-- 
  I won't rest till it's the best ...
  Programmer, Linux Scalability
  Paul Jackson <[EMAIL PROTECTED]> 1.650.933.1373, 
1.925.600.0401
-
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/

Re: [PATCH][1/2] SquashFS

[Phillip Lougher]

Matt Mackall wrote:
On Tue, Mar 15, 2005 at 03:50:26PM +, Phillip Lougher wrote:
Paul Jackson wrote:
In the overall kernel (Linus's bk tree) I count:
733 lines matching 'for *( *; *; *)'
718 lines matching 'while *( *1 *)'
In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
'while(1)' style.
Certainly the 'for(;;)' style is acceptable, and even slightly to
substantially dominant, depending on which piece of code you're in.
I prefer the 'while' style, and only used 'for' because that's what I 
thought the kernel used.

If no-one objects I'll change it back to while...
Shouldn't issues like this be in the coding style document?

This particular point is rather trivial. Do whatever suits you.
It's a shame the 'rather trivial' issue got picked up in the first place 
then :-)  I didn't raise the issue.

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

Re: [PATCH][1/2] SquashFS

[Matt Mackall]

On Tue, Mar 15, 2005 at 03:50:26PM +, Phillip Lougher wrote:
> Paul Jackson wrote:
> >In the overall kernel (Linus's bk tree) I count:
> >
> > 733 lines matching 'for *( *; *; *)'
> > 718 lines matching 'while *( *1 *)'
> >
> >In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
> >'while(1)' style.
> >
> >Certainly the 'for(;;)' style is acceptable, and even slightly to
> >substantially dominant, depending on which piece of code you're in.
> >
> 
> I prefer the 'while' style, and only used 'for' because that's what I 
> thought the kernel used.
> 
> If no-one objects I'll change it back to while...
> 
> Shouldn't issues like this be in the coding style document?

This particular point is rather trivial. Do whatever suits you.

-- 
Mathematics is the supreme nostalgia of our time.
-
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/

Re: [PATCH][1/2] SquashFS

[Phillip Lougher]

Paul Jackson wrote:
In the overall kernel (Linus's bk tree) I count:
733 lines matching 'for *( *; *; *)'
718 lines matching 'while *( *1 *)'
In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
'while(1)' style.
Certainly the 'for(;;)' style is acceptable, and even slightly to
substantially dominant, depending on which piece of code you're in.
I prefer the 'while' style, and only used 'for' because that's what I 
thought the kernel used.

If no-one objects I'll change it back to while...
Shouldn't issues like this be in the coding style document?
Phillip
-
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/

Re: [PATCH][1/2] SquashFS

[Paul Jackson]

In the overall kernel (Linus's bk tree) I count:

733 lines matching 'for *( *; *; *)'
718 lines matching 'while *( *1 *)'

In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
'while(1)' style.

Certainly the 'for(;;)' style is acceptable, and even slightly to
substantially dominant, depending on which piece of code you're in.

-- 
  I won't rest till it's the best ...
  Programmer, Linux Scalability
  Paul Jackson <[EMAIL PROTECTED]> 1.650.933.1373, 
1.925.600.0401
-
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/

Re: [PATCH][1/2] SquashFS

[Paul Jackson]

In the overall kernel (Linus's bk tree) I count:

733 lines matching 'for *( *; *; *)'
718 lines matching 'while *( *1 *)'

In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
'while(1)' style.

Certainly the 'for(;;)' style is acceptable, and even slightly to
substantially dominant, depending on which piece of code you're in.

-- 
  I won't rest till it's the best ...
  Programmer, Linux Scalability
  Paul Jackson [EMAIL PROTECTED] 1.650.933.1373, 
1.925.600.0401
-
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/

Re: [PATCH][1/2] SquashFS

[Phillip Lougher]

Paul Jackson wrote:
In the overall kernel (Linus's bk tree) I count:
733 lines matching 'for *( *; *; *)'
718 lines matching 'while *( *1 *)'
In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
'while(1)' style.
Certainly the 'for(;;)' style is acceptable, and even slightly to
substantially dominant, depending on which piece of code you're in.
I prefer the 'while' style, and only used 'for' because that's what I 
thought the kernel used.

If no-one objects I'll change it back to while...
Shouldn't issues like this be in the coding style document?
Phillip
-
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/

Re: [PATCH][1/2] SquashFS

[Matt Mackall]

On Tue, Mar 15, 2005 at 03:50:26PM +, Phillip Lougher wrote:
 Paul Jackson wrote:
 In the overall kernel (Linus's bk tree) I count:
 
  733 lines matching 'for *( *; *; *)'
  718 lines matching 'while *( *1 *)'
 
 In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
 'while(1)' style.
 
 Certainly the 'for(;;)' style is acceptable, and even slightly to
 substantially dominant, depending on which piece of code you're in.
 
 
 I prefer the 'while' style, and only used 'for' because that's what I 
 thought the kernel used.
 
 If no-one objects I'll change it back to while...
 
 Shouldn't issues like this be in the coding style document?

This particular point is rather trivial. Do whatever suits you.

-- 
Mathematics is the supreme nostalgia of our time.
-
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/

Re: [PATCH][1/2] SquashFS

[Phillip Lougher]

Matt Mackall wrote:
On Tue, Mar 15, 2005 at 03:50:26PM +, Phillip Lougher wrote:
Paul Jackson wrote:
In the overall kernel (Linus's bk tree) I count:
733 lines matching 'for *( *; *; *)'
718 lines matching 'while *( *1 *)'
In the kernel/*.c files, I count 15 of the 'for(;;)' style and 1 of the
'while(1)' style.
Certainly the 'for(;;)' style is acceptable, and even slightly to
substantially dominant, depending on which piece of code you're in.
I prefer the 'while' style, and only used 'for' because that's what I 
thought the kernel used.

If no-one objects I'll change it back to while...
Shouldn't issues like this be in the coding style document?

This particular point is rather trivial. Do whatever suits you.
It's a shame the 'rather trivial' issue got picked up in the first place 
then :-)  I didn't raise the issue.

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

Re: [PATCH][1/2] SquashFS

[Paul Jackson]

 Shouldn't issues like this be in the coding style document?

There is not a concensus (nor a King Penguin dictate) between the
while(1) and for(;;) style to document.  If this were a
frequently asked question, I suppose someone would eventually
note in a coding style doc that either style is acceptable.


 It's a shame the 'rather trivial' issue got picked up in the first place 

Not a shame at all.  Such coding style issues are well known to be a
proper subject for discussion here.

-- 
  I won't rest till it's the best ...
  Programmer, Linux Scalability
  Paul Jackson [EMAIL PROTECTED] 1.650.933.1373, 
1.925.600.0401
-
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/

Re: [PATCH][1/2] SquashFS

[Junio C Hamano]

 PJ == Paul Jackson [EMAIL PROTECTED] writes:

PJ There is not a concensus (nor a King Penguin dictate) between the
PJ while(1) and for(;;) style to document.

FWIW, linux-0.01 has four uses of while (1) and two uses of
for (;;) ;-).

./fs/inode.c:   while (1) {
./fs/namei.c:   while (1) {
./fs/namei.c:   while (1) {
./kernel/sched.c:   while (1) {

./init/main.c:  for(;;) pause();
./kernel/panic.c:   for(;;);

What is interesting here is that the King Penguin used these two
constructs with consistency.  The while (1) form was used with
normal exit routes with if (...) break inside; while the
for(;;) form was used only in unusual the thread of control
should get stuck here forever cases.

So, Phillip's decision to go back to his original while(1) style
seems to be in line with the style used in the original Linux
kernel ;-).

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

Re: [PATCH][1/2] SquashFS

[Paul Jackson]

  the King Penguin used these two constructs with consistency:

Nice distinction - thanks.

-- 
  I won't rest till it's the best ...
  Programmer, Linux Scalability
  Paul Jackson [EMAIL PROTECTED] 1.650.933.1373, 
1.925.600.0401
-
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/

Re: [PATCH][1/2] SquashFS

[Matt Mackall]

On Tue, Mar 15, 2005 at 12:47:23PM +1100, Nick Piggin wrote:
> Matt Mackall wrote:
> 
> >>+   for (;;) {
> >
> >while (1)
> 
> I always thought for (;;) was preferred. Or at least acceptable?

The for (;;) form has always struck me as needlessly clever and I've
known it to puzzle coworkers. I try to make my for loops fall into the
mold of simple initialize/test/advance. But no, I'm not aware of any
LKML concensus opinion on this particular point.

The assignment-in-if problem is a bit more serious as it exacerbates
the jammed-up-against-the-right-margin formatting issues.

-- 
Mathematics is the supreme nostalgia of our time.
-
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/

Re: [PATCH][1/2] SquashFS

[Nick Piggin]

Matt Mackall wrote:
+   for (;;) {
while (1)

I always thought for (;;) was preferred. Or at least acceptable?

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

Re: [PATCH][1/2] SquashFS

[Andrew Morton]

Phillip Lougher <[EMAIL PROTECTED]> wrote:
>
> Please apply the following two patches which adds SquashFS to the
> kernel.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "squashfs.h"
> +

We normally put aam includes after linux includes:

#include 
#include 

#include 
#Include 

> +
> +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 *) )[1] = *((unsigned char *)
> + (bh->b_data + *offset));
> + else
> + ((unsigned char *) )[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 *) )[0] = *((unsigned char *)
> + bh->b_data); 
> + else
> + ((unsigned char *) )[1] = *((unsigned char *)
> + bh->b_data); 
> + *c_byte = temp;
> + *offset = 1;
> + } else {
> + if (msBlk->swap) {
> + ((unsigned char *) )[1] = *((unsigned char *)
> + (bh->b_data + *offset));
> + ((unsigned char *) )[0] = *((unsigned char *)
> + (bh->b_data + *offset + 1)); 
> + } else {
> + ((unsigned char *) )[0] = *((unsigned char *)
> + (bh->b_data + *offset));
> + ((unsigned char *) )[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(, current);
> + add_wait_queue(>waitq, );
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + up(>block_cache_mutex);
> +

Re: [PATCH][1/2] SquashFS

[Matt Mackall]

A quick skim...

> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * inode.c
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "squashfs.h"

Please put all the asm/ bits after all the linux/ bits.
> +
> +static void squashfs_put_super(struct super_block *);
> +static int squashfs_statfs(struct super_block *, struct kstatfs *);
> +static int squashfs_symlink_readpage(struct file *file, struct page *page);
> +static int squashfs_readpage(struct file *file, struct page *page);
> +static int squashfs_readpage4K(struct file *file, struct page *page);
> +static int squashfs_readdir(struct file *, void *, filldir_t);
> +static void squashfs_put_super(struct super_block *s);
> +static struct inode *squashfs_alloc_inode(struct super_block *sb);
> +static void squashfs_destroy_inode(struct inode *inode);
> +static int init_inodecache(void);
> +static void destroy_inodecache(void);
> +static struct dentry *squashfs_lookup(struct inode *, struct dentry *,
> + struct nameidata *);
> +static struct inode *squashfs_iget(struct super_block *s, squashfs_inode 
> inode);
> +static unsigned int read_blocklist(struct inode *inode, int index,
> + int readahead_blks, char *block_list,
> + unsigned short **block_p, unsigned int *bsize);
> +static struct super_block *squashfs_get_sb(struct file_system_type *, int,
> + const char *, void *);

Would be nice to reorder things so that fewer forward declarations
were needed.

> +static z_stream stream;
> +
> +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
> + };

Weird whitespace.

> +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;

Needless cast from void *. Mixed case identifiers are discouraged.

> + if (!(bh = sb_bread(s, *cur_index)))
> + return NULL;

Please don't do assignment inside if().

> + if (msBlk->devblksize - *offset == 1) {
> + if (msBlk->swap)
> + ((unsigned char *) )[1] = *((unsigned char *)
> + (bh->b_data + *offset));
> + else
> + ((unsigned char *) )[0] = *((unsigned char *)
> + (bh->b_data + *offset));

That's rather ugly, what's going on here? There seems to be a lot of
this swapping going on. At the very least, let's use u8.

> +block_release:
> + while (--b >= 0) brelse(bh[b]);

Linebreak.

> + for (i = msBlk->next_cache, n = SQUASHFS_CACHED_BLKS;
> + n ; n --, i = (i + 1) %
> + SQUASHFS_CACHED_BLKS)

Messy. Do n-- (no space), handle i outside the for control structures.
Perhaps break this piece out into a separate function, the indenting
is making things cramped.

> + if (n == 0) {
> + wait_queue_t wait;
> +
> + init_waitqueue_entry(, current);
> + add_wait_queue(>waitq, );
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + up(>block_cache_mutex);
> + schedule();
> + set_current_state(TASK_RUNNING);
> + remove_wait_queue(>waitq, );

I suspect you'll find there's a much cleaner way to do whatever it is you're
trying to do here.

> + if (!(msBlk->block_cache[i].data =
> + (unsigned char *)
> + kmalloc(SQUASHFS_METADATA_SIZE,
> + GFP_KERNEL))) {

Another class of unnecessary cast.

> + msBlk->fragment_index[SQUASHFS_FRAGMENT_INDEX(fragment)];
> + int offset = SQUASHFS_FRAGMENT_INDEX_OFFSET(fragment);

Feel free to make these defines a little less unwieldy. So long as
they're internal to Squashfs.

> + for (;;) {

while (1)

> + for (i = 0; i < SQUASHFS_CACHED_FRAGMENTS &&
> + msBlk->fragment[i].block != start_block; i++);

';' on its own line. Better is 

for (i = 0; i < SQUASHFS_CACHED_FRAGMENTS; i++)

[PATCH][1/2] SquashFS

[Phillip Lougher]

Andrew, Greg,
Please apply the following two patches which adds SquashFS to the
kernel.  SquashFS is self contained compressed filesystem, and it
has been used by a large amount of projects over the last few years
without problems.
A number of people have started to ask me to submit it to the kernel.
Please consider adding it.
The SquashFS patch has been split into two.  This email contains
inode.c, the next email contains everything else.
Thanks
Phillip Lougher
Signed-Off-By: Phillip Lougher ([EMAIL PROTECTED])

diff --new-file -urp linux-2.6.11.3/fs/squashfs/inode.c 
linux-2.6.11.3-squashfs/fs/squashfs/inode.c
--- linux-2.6.11.3/fs/squashfs/inode.c  1970-01-01 01:00:00.0 +0100
+++ linux-2.6.11.3-squashfs/fs/squashfs/inode.c 2005-03-14 00:53:28.037568088 
+
@@ -0,0 +1,1784 @@
+/*
+ * Squashfs - a compressed read only filesystem for Linux
+ *
+ * Copyright (c) 2002, 2003, 2004, 2005
+ * Phillip Lougher <[EMAIL PROTECTED]>
+ *
+ * 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, 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * inode.c
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "squashfs.h"
+
+static void squashfs_put_super(struct super_block *);
+static int squashfs_statfs(struct super_block *, struct kstatfs *);
+static int squashfs_symlink_readpage(struct file *file, struct page *page);
+static int squashfs_readpage(struct file *file, struct page *page);
+static int squashfs_readpage4K(struct file *file, struct page *page);
+static int squashfs_readdir(struct file *, void *, filldir_t);
+static void squashfs_put_super(struct super_block *s);
+static struct inode *squashfs_alloc_inode(struct super_block *sb);
+static void squashfs_destroy_inode(struct inode *inode);
+static int init_inodecache(void);
+static void destroy_inodecache(void);
+static struct dentry *squashfs_lookup(struct inode *, struct dentry *,
+   struct nameidata *);
+static struct inode *squashfs_iget(struct super_block *s, squashfs_inode 
inode);
+static unsigned int read_blocklist(struct inode *inode, int index,
+   int readahead_blks, char *block_list,
+   unsigned short **block_p, unsigned int *bsize);
+static struct super_block *squashfs_get_sb(struct file_system_type *, int,
+   const char *, void *);
+
+DECLARE_MUTEX(read_data_mutex);
+
+static z_stream stream;
+
+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
+   };
+
+static unsigned char squashfs_filetype_table[] = {
+   DT_UNKNOWN, DT_DIR, DT_REG, DT_LNK, DT_BLK, DT_CHR, DT_FIFO, DT_SOCK
+};
+
+static struct super_operations squashfs_ops = {
+   .alloc_inode = squashfs_alloc_inode,
+   .destroy_inode = squashfs_destroy_inode,
+   .statfs = squashfs_statfs,
+   .put_super = squashfs_put_super,
+};
+
+struct address_space_operations squashfs_symlink_aops = {
+   .readpage = squashfs_symlink_readpage
+};
+
+struct address_space_operations squashfs_aops = {
+   .readpage = squashfs_readpage
+};
+
+struct address_space_operations squashfs_aops_4K = {
+   .readpage = squashfs_readpage4K
+};
+
+struct file_operations squashfs_dir_ops = {
+   .read = generic_read_dir,
+   .readdir = squashfs_readdir
+};
+
+struct inode_operations squashfs_dir_inode_ops = {
+   .lookup = squashfs_lookup
+};
+
+
+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;
+   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 *) )[1] = *((unsigned char *)
+   (bh->b_data + *offset));
+   else
+   ((unsigned char *) )[0] = *((unsigned char *)
+  

[PATCH][1/2] SquashFS

[Phillip Lougher]

Andrew, Greg,
Please apply the following two patches which adds SquashFS to the
kernel.  SquashFS is self contained compressed filesystem, and it
has been used by a large amount of projects over the last few years
without problems.
A number of people have started to ask me to submit it to the kernel.
Please consider adding it.
The SquashFS patch has been split into two.  This email contains
inode.c, the next email contains everything else.
Thanks
Phillip Lougher
Signed-Off-By: Phillip Lougher ([EMAIL PROTECTED])

diff --new-file -urp linux-2.6.11.3/fs/squashfs/inode.c 
linux-2.6.11.3-squashfs/fs/squashfs/inode.c
--- linux-2.6.11.3/fs/squashfs/inode.c  1970-01-01 01:00:00.0 +0100
+++ linux-2.6.11.3-squashfs/fs/squashfs/inode.c 2005-03-14 00:53:28.037568088 
+
@@ -0,0 +1,1784 @@
+/*
+ * Squashfs - a compressed read only filesystem for Linux
+ *
+ * Copyright (c) 2002, 2003, 2004, 2005
+ * Phillip Lougher [EMAIL PROTECTED]
+ *
+ * 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, 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * inode.c
+ */
+
+#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
+
+static void squashfs_put_super(struct super_block *);
+static int squashfs_statfs(struct super_block *, struct kstatfs *);
+static int squashfs_symlink_readpage(struct file *file, struct page *page);
+static int squashfs_readpage(struct file *file, struct page *page);
+static int squashfs_readpage4K(struct file *file, struct page *page);
+static int squashfs_readdir(struct file *, void *, filldir_t);
+static void squashfs_put_super(struct super_block *s);
+static struct inode *squashfs_alloc_inode(struct super_block *sb);
+static void squashfs_destroy_inode(struct inode *inode);
+static int init_inodecache(void);
+static void destroy_inodecache(void);
+static struct dentry *squashfs_lookup(struct inode *, struct dentry *,
+   struct nameidata *);
+static struct inode *squashfs_iget(struct super_block *s, squashfs_inode 
inode);
+static unsigned int read_blocklist(struct inode *inode, int index,
+   int readahead_blks, char *block_list,
+   unsigned short **block_p, unsigned int *bsize);
+static struct super_block *squashfs_get_sb(struct file_system_type *, int,
+   const char *, void *);
+
+DECLARE_MUTEX(read_data_mutex);
+
+static z_stream stream;
+
+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
+   };
+
+static unsigned char squashfs_filetype_table[] = {
+   DT_UNKNOWN, DT_DIR, DT_REG, DT_LNK, DT_BLK, DT_CHR, DT_FIFO, DT_SOCK
+};
+
+static struct super_operations squashfs_ops = {
+   .alloc_inode = squashfs_alloc_inode,
+   .destroy_inode = squashfs_destroy_inode,
+   .statfs = squashfs_statfs,
+   .put_super = squashfs_put_super,
+};
+
+struct address_space_operations squashfs_symlink_aops = {
+   .readpage = squashfs_symlink_readpage
+};
+
+struct address_space_operations squashfs_aops = {
+   .readpage = squashfs_readpage
+};
+
+struct address_space_operations squashfs_aops_4K = {
+   .readpage = squashfs_readpage4K
+};
+
+struct file_operations squashfs_dir_ops = {
+   .read = generic_read_dir,
+   .readdir = squashfs_readdir
+};
+
+struct inode_operations squashfs_dir_inode_ops = {
+   .lookup = squashfs_lookup
+};
+
+
+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;
+   unsigned short temp;
+   struct buffer_head *bh;
+
+   if (!(bh = sb_bread(s, *cur_index)))
+   return NULL;
+
+   if (msBlk-devblksize - *offset == 

Re: [PATCH][1/2] SquashFS

[Matt Mackall]

A quick skim...

 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
 + *
 + * inode.c
 + */
 +
 +#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

Please put all the asm/ bits after all the linux/ bits.
 +
 +static void squashfs_put_super(struct super_block *);
 +static int squashfs_statfs(struct super_block *, struct kstatfs *);
 +static int squashfs_symlink_readpage(struct file *file, struct page *page);
 +static int squashfs_readpage(struct file *file, struct page *page);
 +static int squashfs_readpage4K(struct file *file, struct page *page);
 +static int squashfs_readdir(struct file *, void *, filldir_t);
 +static void squashfs_put_super(struct super_block *s);
 +static struct inode *squashfs_alloc_inode(struct super_block *sb);
 +static void squashfs_destroy_inode(struct inode *inode);
 +static int init_inodecache(void);
 +static void destroy_inodecache(void);
 +static struct dentry *squashfs_lookup(struct inode *, struct dentry *,
 + struct nameidata *);
 +static struct inode *squashfs_iget(struct super_block *s, squashfs_inode 
 inode);
 +static unsigned int read_blocklist(struct inode *inode, int index,
 + int readahead_blks, char *block_list,
 + unsigned short **block_p, unsigned int *bsize);
 +static struct super_block *squashfs_get_sb(struct file_system_type *, int,
 + const char *, void *);

Would be nice to reorder things so that fewer forward declarations
were needed.

 +static z_stream stream;
 +
 +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
 + };

Weird whitespace.

 +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;

Needless cast from void *. Mixed case identifiers are discouraged.

 + if (!(bh = sb_bread(s, *cur_index)))
 + return NULL;

Please don't do assignment inside if().

 + 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));

That's rather ugly, what's going on here? There seems to be a lot of
this swapping going on. At the very least, let's use u8.

 +block_release:
 + while (--b = 0) brelse(bh[b]);

Linebreak.

 + for (i = msBlk-next_cache, n = SQUASHFS_CACHED_BLKS;
 + n ; n --, i = (i + 1) %
 + SQUASHFS_CACHED_BLKS)

Messy. Do n-- (no space), handle i outside the for control structures.
Perhaps break this piece out into a separate function, the indenting
is making things cramped.

 + 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);

I suspect you'll find there's a much cleaner way to do whatever it is you're
trying to do here.

 + if (!(msBlk-block_cache[i].data =
 + (unsigned char *)
 + kmalloc(SQUASHFS_METADATA_SIZE,
 + GFP_KERNEL))) {

Another class of unnecessary cast.

 + msBlk-fragment_index[SQUASHFS_FRAGMENT_INDEX(fragment)];
 + int offset = SQUASHFS_FRAGMENT_INDEX_OFFSET(fragment);

Feel free to make these defines a little less unwieldy. So long as
they're internal to Squashfs.

 + for (;;) {

while (1)

 + for (i = 0; 

Re: [PATCH][1/2] SquashFS

[Andrew Morton]

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;
 +
 + 

Re: [PATCH][1/2] SquashFS

[Nick Piggin]

Matt Mackall wrote:
+   for (;;) {
while (1)

I always thought for (;;) was preferred. Or at least acceptable?

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

Re: [PATCH][1/2] SquashFS

[Matt Mackall]

On Tue, Mar 15, 2005 at 12:47:23PM +1100, Nick Piggin wrote:
 Matt Mackall wrote:
 
 +   for (;;) {
 
 while (1)
 
 I always thought for (;;) was preferred. Or at least acceptable?

The for (;;) form has always struck me as needlessly clever and I've
known it to puzzle coworkers. I try to make my for loops fall into the
mold of simple initialize/test/advance. But no, I'm not aware of any
LKML concensus opinion on this particular point.

The assignment-in-if problem is a bit more serious as it exacerbates
the jammed-up-against-the-right-margin formatting issues.

-- 
Mathematics is the supreme nostalgia of our time.
-
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/