Modularize xz.

[kib]

Summary

Embedded lzma decompression library becomes a module usable by other consumers, in addition to geom_uzip.

Most important code changes are

• removal of XZ_DEC_SINGLE define, we need the code to work with XZ_DEC_DYNALLOC
• xz_crc32_init() call is removed from geom_uzip, xz module handles initialization on its own.

xz is no longer embedded into geom_uzip, instead the depend line for the module is provided, and corresponding kernel option is added to each MIPS kernel config file using geom_uzip.

Sponsored by: Mellanox Technologies

[• hselasky]

Looks good to me.

[ray]

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

[kib]

In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

[slavash]

Looks good to me.

[• hselasky]

In D19266#412263, @kib wrote:

In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

I think he meant the kernel option name.

[ray]

In D19266#412263, @kib wrote:

In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

device xz_embedded

[kib]

In D19266#412271, @ray wrote:

In D19266#412263, @kib wrote:

In D19266#412252, @ray wrote:

Looks fine!
Cosmetic: why not to name it just "xz" instead of "xz_embedded"?

Both module, module build directory, and linker file are named xz. What part name don't you like ?

device xz_embedded

This is how it called now. Look at sys/conf/files, where xz is under xz_embedded. In fact there is a missed bit of the patch, I will rename it to xz together with the rename.

[kib]

Actually include the chunk which removes xz files from under geom_uzip when compiled into kernel.
Rename xz_embedded to xz, as requested by ray.

[ray]

Thanks!

[sobomax]

Looks good overall. I am little bit puzzled over usage of "option GEOM_UZIP" vs. "device geom_uzip + device xz". I suspect those are functionally equivalent. Half of the kernel configs in the tree use former (including NOTES) and half the latter. Some subset of the two use both. :( Since you are touching those files anyways, perhaps you can go and convert all cases of "device geom_uzip" into plain "option GEOM_UZIP" and not patch it up by adding device xz? It might be also a good idea to adjust geom_uzip(4) accordingly to provide "One-True-Way" to compile geom_uzip(4) statically into the kernel? Thanks!

[sobomax]

Also "device xz" needs to be added to the conf/NOTES.

[kib]

In D19266#412536, @sobomax wrote:

Looks good overall. I am little bit puzzled over usage of "option GEOM_UZIP" vs. "device geom_uzip + device xz". I suspect those are functionally equivalent. Half of the kernel configs in the tree use former (including NOTES) and half the latter. Some subset of the two use both. :( Since you are touching those files anyways, perhaps you can go and convert all cases of "device geom_uzip" into plain "option GEOM_UZIP" and not patch it up by adding device xz? It might be also a good idea to adjust geom_uzip(4) accordingly to provide "One-True-Way" to compile geom_uzip(4) statically into the kernel? Thanks!

option GEOM_UZIP/device geom_uzip has nothing to do with my patch, this redundancy is already present and cleaning it up is unrelated. Apparently the same redundancy is there with GEOM_MAP. It seems that all geom classes added using options and not device. Also I do not see it would allow me to avoid adding device xz.

I removed device geom_uzip from configs which I touch, but really I should not.

[kib]

Remove 'device geom_uzip', add xz to notes.

[sobomax]

In D19266#412644, @kib wrote:

Remove 'device geom_uzip', add xz to notes.

Thanks for cleaning the mess, it looks better now however some further work is needed I think to either document new dependency in the share/man/man4/geom_uzip.4 or maybe get rid of the need to explicitly specify "device xz" when option GEOM_UZIP is given? geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different? For once xz(4) has no other consumers in the kernel yet, while zlib certainly does, so it should be the reference model IMHO.

[sobomax@pioneer ~/projects/FreeBSD/head]$ find sys/ -type f | xargs grep -w zlib | grep MODULE_DEPEND
sys/netgraph/ng_deflate.c:MODULE_DEPEND(ng_deflate, zlib, 1, 1, 1);
sys/dev/mxge/if_mxge.c:MODULE_DEPEND(mxge, zlib, 1, 1, 1);
sys/geom/uzip/g_uzip.c:MODULE_DEPEND(g_uzip, zlib, 1, 1, 1);
sys/opencrypto/crypto.c:MODULE_DEPEND(crypto, zlib, 1, 1, 1);
sys/opencrypto/cryptodev.c:MODULE_DEPEND(cryptodev, zlib, 1, 1, 1);

[cem]

In D19266#412904, @sobomax wrote:

geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different?

zlib is built if geom_uzip is enabled in conf/files:

libkern/zlib.c                  optional crypto | geom_uzip | ipsec | \
        ipsec_support | mxge | netgraph_deflate | ddb_ctf | gzio

(Also, zlib has many other consumers. xz-embedded has limited general utility, due to being a decompress-only subset of lzma. So I am not sure xz will gain many more consumers. I agree adding 'device xz' to a million MIPS kernel configurations is an ugly repercussion of this choice.)

[kib]

In D19266#412909, @cem wrote:

In D19266#412904, @sobomax wrote:

geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different?

zlib is built if geom_uzip is enabled in conf/files:

libkern/zlib.c                  optional crypto | geom_uzip | ipsec | \
        ipsec_support | mxge | netgraph_deflate | ddb_ctf | gzio

(Also, zlib has many other consumers. xz-embedded has limited general utility, due to being a decompress-only subset of lzma. So I am not sure xz will gain many more consumers. I agree adding 'device xz' to a million MIPS kernel configurations is an ugly repercussion of this choice.)

What you cited is clear example why I (and others) do not do this dependency tracking insanity in the conf/files syntax. Config(8) is only effective at flat dependencies where all required config options are explicitly provided in the kernel config file. Tracking dependencies like turn on B when A is on, but also consider side effects from C, D, E, architecture, and so on, is impossible with it.

I thought it is obvious that the only reason why I wrote this patch is because there are more planned consumers of xz in kernel, which alone is the reason why I want it to be a dedicated option, and why I want (need) to avoid the dependency drama.

Also see the recent discussion of iflib modularization.

That said, are there any other _technical_ notes about the diff ?

[sobomax]

In D19266#412957, @kib wrote:

In D19266#412909, @cem wrote:

In D19266#412904, @sobomax wrote:

geom_uzip(4) uses zlib module without the need to add dummy "device zlib" into kernel, why xz code should be any different?

zlib is built if geom_uzip is enabled in conf/files:

libkern/zlib.c                  optional crypto | geom_uzip | ipsec | \
        ipsec_support | mxge | netgraph_deflate | ddb_ctf | gzio

(Also, zlib has many other consumers. xz-embedded has limited general utility, due to being a decompress-only subset of lzma. So I am not sure xz will gain many more consumers. I agree adding 'device xz' to a million MIPS kernel configurations is an ugly repercussion of this choice.)

What you cited is clear example why I (and others) do not do this dependency tracking insanity in the conf/files syntax. Config(8) is only effective at flat dependencies where all required config options are explicitly provided in the kernel config file. Tracking dependencies like turn on B when A is on, but also consider side effects from C, D, E, architecture, and so on, is impossible with it.

I thought it is obvious that the only reason why I wrote this patch is because there are more planned consumers of xz in kernel, which alone is the reason why I want it to be a dedicated option, and why I want (need) to avoid the dependency drama.

Also see the recent discussion of iflib modularization.

That said, are there any other _technical_ notes about the diff ?

Yes, you've missed my yesterday's request to correct synopsis of the static option GEOM_UZIP compilation in the geom_uzip.4 manual page in case you want to make the "device xz" necessary. See geli(8) for example:

SYNOPSIS
     To compile GEOM_ELI into your kernel, add the following lines to your
     kernel configuration file:

           device crypto
           options GEOM_ELI

Other than that I don't have any major technical problems with the patch, assuming everyone else agrees that needing to have "device xz" is a necessary evil. There are some minor things I'd leave as inline comments though.

[sobomax]

sys/conf/NOTES

3004 ↗

(On Diff #54180)

compression -> de-compression

[sobomax]

sys/contrib/xz-embedded/freebsd/xz_malloc.c

54 ↗

(On Diff #54180)

I don't think this function and all things below (moduledata_t xz_moduledata etc) belongs to the xz_malloc.c. It clearly has nothing to do with "malloc". I'd suggest something like xz_mod.c or something.

[sobomax]

sys/contrib/xz-embedded/freebsd/xz_malloc.c

54 ↗

(On Diff #54180)

P.S. xz_mod.c (xz.c?) itself should probably live in the sys/dev/xz, like most other FreeBSD-specific device code. It is not contributed in any way and I don't think anyone but us would care to maintain it.

[sobomax]

sys/modules/xz/Makefile

6 ↗

(On Diff #54180)

KMOD=xz -> KMOD=<tab>xz

[kib]

Rebase; fix conflicts.
Formatting and comment changes.

[sobomax]

In D19266#413225, @kib wrote:

Rebase; fix conflicts.
Formatting and comment changes.

Some of technical issues that I raised remain unaddressed (documentation, module-specific code placement). So the diff still needs bit more work IMHO.