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 ?

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.

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

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.

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

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.

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.)

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 ?

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.