2010-12-09 06:11:39

by Phillip Lougher

[permalink] [raw]
Subject: [PATCH 2/2] Squashfs: Add XZ compression configuration option


Signed-off-by: Phillip Lougher <[email protected]>
---
fs/squashfs/Kconfig | 16 ++++++++++++++++
fs/squashfs/Makefile | 1 +
fs/squashfs/decompressor.c | 11 +++++++++++
fs/squashfs/squashfs.h | 3 +++
4 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig
index e5f63da..e96d99a 100644
--- a/fs/squashfs/Kconfig
+++ b/fs/squashfs/Kconfig
@@ -53,6 +53,22 @@ config SQUASHFS_LZO

If unsure, say N.

+config SQUASHFS_XZ
+ bool "Include support for XZ compressed file systems"
+ depends on SQUASHFS
+ default n
+ select XZ_DEC
+ help
+ Saying Y here includes support for reading Squashfs file systems
+ compressed with XZ compresssion. XZ gives better compression than
+ the default zlib compression, at the expense of greater CPU and
+ memory overhead.
+
+ XZ is not the standard compression used in Squashfs and so most
+ file systems will be readable without selecting this option.
+
+ If unsure, say N.
+
config SQUASHFS_EMBEDDED
bool "Additional option for memory-constrained systems"
depends on SQUASHFS
diff --git a/fs/squashfs/Makefile b/fs/squashfs/Makefile
index 7672bac..cecf2be 100644
--- a/fs/squashfs/Makefile
+++ b/fs/squashfs/Makefile
@@ -7,3 +7,4 @@ squashfs-y += block.o cache.o dir.o export.o file.o fragment.o id.o inode.o
squashfs-y += namei.o super.o symlink.o zlib_wrapper.o decompressor.o
squashfs-$(CONFIG_SQUASHFS_XATTR) += xattr.o xattr_id.o
squashfs-$(CONFIG_SQUASHFS_LZO) += lzo_wrapper.o
+squashfs-$(CONFIG_SQUASHFS_XZ) += xz_wrapper.o
diff --git a/fs/squashfs/decompressor.c b/fs/squashfs/decompressor.c
index 24af9ce..ac333b8 100644
--- a/fs/squashfs/decompressor.c
+++ b/fs/squashfs/decompressor.c
@@ -46,6 +46,12 @@ static const struct squashfs_decompressor squashfs_lzo_unsupported_comp_ops = {
};
#endif

+#ifndef CONFIG_SQUASHFS_XZ
+static const struct squashfs_decompressor squashfs_xz_unsupported_comp_ops = {
+ NULL, NULL, NULL, XZ_COMPRESSION, "xz", 0
+};
+#endif
+
static const struct squashfs_decompressor squashfs_unknown_comp_ops = {
NULL, NULL, NULL, 0, "unknown", 0
};
@@ -58,6 +64,11 @@ static const struct squashfs_decompressor *decompressor[] = {
#else
&squashfs_lzo_unsupported_comp_ops,
#endif
+#ifdef CONFIG_SQUASHFS_XZ
+ &squashfs_xz_comp_ops,
+#else
+ &squashfs_xz_unsupported_comp_ops,
+#endif
&squashfs_unknown_comp_ops
};

diff --git a/fs/squashfs/squashfs.h b/fs/squashfs/squashfs.h
index 5d45569..1096e2e 100644
--- a/fs/squashfs/squashfs.h
+++ b/fs/squashfs/squashfs.h
@@ -107,3 +107,6 @@ extern const struct squashfs_decompressor squashfs_zlib_comp_ops;

/* lzo_wrapper.c */
extern const struct squashfs_decompressor squashfs_lzo_comp_ops;
+
+/* xz_wrapper.c */
+extern const struct squashfs_decompressor squashfs_xz_comp_ops;
--
1.6.3.3


2010-12-09 07:02:09

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 2/2] Squashfs: Add XZ compression configuration option

On Thu, Dec 9, 2010 at 07:11, Phillip Lougher
<[email protected]> wrote:
> --- a/fs/squashfs/Kconfig
> +++ b/fs/squashfs/Kconfig
> @@ -53,6 +53,22 @@ config SQUASHFS_LZO
>
>          If unsure, say N.
>
> +config SQUASHFS_XZ
> +       bool "Include support for XZ compressed file systems"
> +       depends on SQUASHFS
> +       default n

"default n" is the default, no reason to specify it.

Do we need a checkpatch test for this?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

2010-12-09 09:09:07

by Lasse Collin

[permalink] [raw]
Subject: Re: [PATCH 2/2] Squashfs: Add XZ compression configuration option

On 2010-12-09 Phillip Lougher wrote:
> +config SQUASHFS_XZ
> + bool "Include support for XZ compressed file systems"
> + depends on SQUASHFS
> + default n
> + select XZ_DEC

Should "select XZ_DEC" be replaced with "depends on XZ_DEC"? XZ_DEC
requires CRC32, so if "select XZ_DEC" is used, there needs to be also
"select CRC32".

XZ_DEC may optionally use other XZ_DEC_* symbols, which the user will
want to choose when building for an embedded system. With "depends on
XZ_DEC" the user will see that there's more than a single option that
affects the details of the XZ support in Squashfs.

--
Lasse Collin | IRC: Larhzu @ IRCnet & Freenode

2010-12-10 06:23:49

by Phillip Lougher

[permalink] [raw]
Subject: Re: [PATCH 2/2] Squashfs: Add XZ compression configuration option

Geert Uytterhoeven wrote:
> On Thu, Dec 9, 2010 at 07:11, Phillip Lougher
> <[email protected]> wrote:
>> --- a/fs/squashfs/Kconfig
>> +++ b/fs/squashfs/Kconfig
>> @@ -53,6 +53,22 @@ config SQUASHFS_LZO
>>
>> If unsure, say N.
>>
>> +config SQUASHFS_XZ
>> + bool "Include support for XZ compressed file systems"
>> + depends on SQUASHFS
>> + default n
>
> "default n" is the default, no reason to specify it.
>

Yes, thanks for pointing that out.

It seems there's a lot of "default n"s in the mainline kernel

% find . -name "Kconfig"| xargs grep "default n" | wc
485 1535 18753

including two in arch/m68k ;-)

Phillip

2010-12-10 07:30:43

by Phillip Lougher

[permalink] [raw]
Subject: Re: [PATCH 2/2] Squashfs: Add XZ compression configuration option

Lasse Collin wrote:
> On 2010-12-09 Phillip Lougher wrote:
>> +config SQUASHFS_XZ
>> + bool "Include support for XZ compressed file systems"
>> + depends on SQUASHFS
>> + default n
>> + select XZ_DEC
>
> Should "select XZ_DEC" be replaced with "depends on XZ_DEC"? XZ_DEC
> requires CRC32, so if "select XZ_DEC" is used, there needs to be also
> "select CRC32".
>

XZ_DEC selects CRC32, kbuild handles these nested selects quite happily,
so if something selects XZ_DEC it knows it has to also select CRC32.

Depends on has quite different semantics to selects. If SQUASHFS_XZ
was made to depend on XZ_DEC then the option simply won't appear unless
the user knew to select XZ_DEC first (as it's default n). This would
prove extremely confusing, and probably lead to most people thinking
Squashfs didn't have XZ support, which is somewhat undesirable.

> XZ_DEC may optionally use other XZ_DEC_* symbols, which the user will
> want to choose when building for an embedded system. With "depends on
> XZ_DEC" the user will see that there's more than a single option that
> affects the details of the XZ support in Squashfs.
>

With depends on XZ_DEC the user will simply not see that Squashfs has
XZ support (as the option won't appear unless XZ_DEC is explicitly
selected).

With selects XZ_DEC users will see that Squashfs has XZ support, if
enabled, they'll simply see that XZ_DEC has been automatically
selected in the "Library routines" sub-menu. If EMBEDDED is not
selected the XZ_DEC options will be automatically selected (as
they're only user selectable if EMBEDDED is selected). If
EMBEDDED is selected, then they'll have the choice then to decide
which options they wish to de-select.

I think this is preferable to needing XZ_DEC to be selected before
the SQUASHFS_XZ option even appears.

Phillip

2010-12-10 10:36:46

by Lasse Collin

[permalink] [raw]
Subject: Re: [PATCH 2/2] Squashfs: Add XZ compression configuration option

On 2010-12-10 Phillip Lougher wrote:
> Lasse Collin wrote:
> > On 2010-12-09 Phillip Lougher wrote:
> >> +config SQUASHFS_XZ
> >> + bool "Include support for XZ compressed file systems"
> >> + depends on SQUASHFS
> >> + default n
> >> + select XZ_DEC
> >
> > Should "select XZ_DEC" be replaced with "depends on XZ_DEC"? XZ_DEC
> > requires CRC32, so if "select XZ_DEC" is used, there needs to be
> > also "select CRC32".
>
> XZ_DEC selects CRC32, kbuild handles these nested selects quite
> happily, so if something selects XZ_DEC it knows it has to also
> select CRC32.

OK, I had misunderstood the notice about "select" and dependencies in
Documentation/kbuild/kconfig-language.txt.

> Depends on has quite different semantics to selects. If SQUASHFS_XZ
> was made to depend on XZ_DEC then the option simply won't appear
> unless the user knew to select XZ_DEC first (as it's default n).
> This would prove extremely confusing, and probably lead to most
> people thinking Squashfs didn't have XZ support, which is somewhat
> undesirable.

Good point. I always keep the inactive options visible in xconfig so I
missed this. I will need to update my XZ boot-time support patch to use
"select" instead of "depends on" too. Thanks!

--
Lasse Collin | IRC: Larhzu @ IRCnet & Freenode