Add Global Config Option for creating User BTRFS Subvolumes in $HOME

[silverhadch]

Fixes #1162

[Conan-Kudo]

Can you squash this into a single commit? And format the commit message properly?

Something like src/useradd: Support config for creating home dirs as Btrfs subvolumes

[alejandro-colomar]

Please add a Closes: tag in the commit message for the github issue it closes.

[silverhadch]

Please add a Closes: tag in the commit message for the github issue it closes.

Done.

[Conan-Kudo]

This needs squashing again. 😄

[Conan-Kudo]

It looks like this project requires DCO signoffs, so you'll want to amend the commit with -s and also add my signoff:

Signed-off-by: Neal Gompa <ngompa@velocitylimitless.com>

Also, can you please update the co-authored-by to: Co-authored-by: Neal Gompa <ngompa@velocitylimitless.com>?

[silverhadch]

@alejandro-colomar
Also I added the small comment to match the rest of the else if blocks.

[alejandro-colomar]

Thanks! LGTM.

Reviewed-by: Alejandro Colomar <alx@kernel.org>

Since it's a new feature, I'd like @hallyn or @ikerexxe to also review this.

Also, we're currently in a freeze, as we'll release in a week. We won't apply the patch until after the release. (I'll open an issue in a random line to make sure it blocks accidentally merging.)

[silverhadch]

Thanks for your time and help with polishing and reviewing the PR :)
You , too @Conan-Kudo !

[silverhadch]

You should also add an explanation for this new option in the man pages, similar to what CREATE_HOME or DEFAULT_HOME have in man/login.defs.d/.

In addition, if you feel comfortable you could also create a test case for this new option.

Finally, as Alejandro has already mentioned, we are about to release a new version and we are focusing on stabilizing it, so probably this will get merged after that.

Wouldn't that require the thing running the test to be on BTRFS and /tmp which I have seen in the test being the Home Directory also being tmpfs and not BTRFS. Technically I could do an BTRFS Loopdevice Image and use that but that seems to be exetremly complex in contrast to other tests.

[alejandro-colomar]

You should also add an explanation for this new option in the man pages, similar to what CREATE_HOME or DEFAULT_HOME have in man/login.defs.d/.
In addition, if you feel comfortable you could also create a test case for this new option.
Finally, as Alejandro has already mentioned, we are about to release a new version and we are focusing on stabilizing it, so probably this will get merged after that.

Wouldn't that require the thing running the test to be on BTRFS and /tmp which I have seen in the test being the Home Directory also being tmpfs and not BTRFS. Technically I could do an BTRFS Loopdevice Image and use that but that seems to be exetremly complex in contrast to other tests.

Being so complex, I'd say just test it on your own computer, and if it passes, that's fine. Please include in the commit message the shell session with which you tested it, so that one can reproduce the test in their computer.

Be aware that we might accidentally break something eventually, since we don't have regression tests, so it would be good if you (plural you, the users of this feature), report when it breaks. I don't use this feature myself, so can't know if it breaks.

If you eventually want to write a test, that would of course be fine too.

[silverhadch]

You should also add an explanation for this new option in the man pages, similar to what CREATE_HOME or DEFAULT_HOME have in man/login.defs.d/.
In addition, if you feel comfortable you could also create a test case for this new option.
Finally, as Alejandro has already mentioned, we are about to release a new version and we are focusing on stabilizing it, so probably this will get merged after that.

Wouldn't that require the thing running the test to be on BTRFS and /tmp which I have seen in the test being the Home Directory also being tmpfs and not BTRFS. Technically I could do an BTRFS Loopdevice Image and use that but that seems to be exetremly complex in contrast to other tests.

Being so complex, I'd say just test it on your own computer, and if it passes, that's fine. Please include in the commit message the shell session with which you tested it, so that one can reproduce the test in their computer.

Be aware that we might accidentally break something eventually, since we don't have regression tests, so it would be good if you (plural you, the users of this feature), report when it breaks. I don't use this feature myself, so can't know if it breaks.

If you eventually want to write a test, that would of course be fine too.

Ahhh I will probably figure something out.

[silverhadch]

You should also add an explanation for this new option in the man pages, similar to what CREATE_HOME or DEFAULT_HOME have in man/login.defs.d/.
In addition, if you feel comfortable you could also create a test case for this new option.
Finally, as Alejandro has already mentioned, we are about to release a new version and we are focusing on stabilizing it, so probably this will get merged after that.

Wouldn't that require the thing running the test to be on BTRFS and /tmp which I have seen in the test being the Home Directory also being tmpfs and not BTRFS. Technically I could do an BTRFS Loopdevice Image and use that but that seems to be exetremly complex in contrast to other tests.

Being so complex, I'd say just test it on your own computer, and if it passes, that's fine. Please include in the commit message the shell session with which you tested it, so that one can reproduce the test in their computer.

Be aware that we might accidentally break something eventually, since we don't have regression tests, so it would be good if you (plural you, the users of this feature), report when it breaks. I don't use this feature myself, so can't know if it breaks.

If you eventually want to write a test, that would of course be fine too.

Well we want to use it in a Distro and Fedora has an proposal etc. So eventually there will be enough Users that will show up when something breaks.

[alejandro-colomar]

You should also add an explanation for this new option in the man pages, similar to what CREATE_HOME or DEFAULT_HOME have in man/login.defs.d/.
In addition, if you feel comfortable you could also create a test case for this new option.
Finally, as Alejandro has already mentioned, we are about to release a new version and we are focusing on stabilizing it, so probably this will get merged after that.

Wouldn't that require the thing running the test to be on BTRFS and /tmp which I have seen in the test being the Home Directory also being tmpfs and not BTRFS. Technically I could do an BTRFS Loopdevice Image and use that but that seems to be exetremly complex in contrast to other tests.

Being so complex, I'd say just test it on your own computer, and if it passes, that's fine. Please include in the commit message the shell session with which you tested it, so that one can reproduce the test in their computer.
Be aware that we might accidentally break something eventually, since we don't have regression tests, so it would be good if you (plural you, the users of this feature), report when it breaks. I don't use this feature myself, so can't know if it breaks.
If you eventually want to write a test, that would of course be fine too.

Well we want to use it in a Distro and Fedora has an proposal etc. So eventually there will be enough Users that will show up when something breaks.

That sounds good. Thanks!

[ikerexxe]

Thanks for the update in the documentation. Now I see why we can't create a specific file for this option.

As a last note, you should guard the code with #ifdef WITH_BTRFS, just like with the command-line option

[alejandro-colomar]

As a last note, you should guard the code with #ifdef WITH_BTRFS, just like with the command-line option

Would that change anything in observable behavior?

[alejandro-colomar]

As a last note, you should guard the code with #ifdef WITH_BTRFS, just like with the command-line option

Would that change anything in observable behavior?

I think this code could be fine without wrapping in cpp(1) conditionals. Please revise, as I'm not 100% sure, though.

[silverhadch]

Thanks for the update in the documentation. Now I see why we can't create a specific file for this option.

As a last note, you should guard the code with #ifdef WITH_BTRFS, just like with the command-line option

Pretty sure having the variable set to true will have zero effects. The Code in this PR only adds the ability to define it in /etc/default/useradd instead of only an cdmline argument. If with BTRFS is disabled the config has zero effect, no?

[silverhadch]

@ikerexxe @alejandro-colomar
I just tested it. It has zero effect if you set the variable when you dont compile with BTRFS Support. It will simply create an regular user. create_home() will simply ignore the variable even if it is set since it cant do anything with it when compiled without BTRFS Support.

[ikerexxe]

To avoid giving users the false impression that btrfs subvolumes are being created, we should conditionally compile the section. If the feature is compiled out, the associated help text and logic won't be visible to the user

[Conan-Kudo]

That's probably out of scope for this PR, since it was never like that before.

[alejandro-colomar]

To avoid giving users the false impression that btrfs subvolumes are being created, we should conditionally compile the section.

On the other hand, there's https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation, which recommends avoiding conditional compilation as much as possible.

If the feature is compiled out, the associated help text and logic won't be visible to the user

By visible text you mean with strings(1)?
https://www.man7.org/linux/man-pages/man1/strings.1.html

[Conan-Kudo]

Also, if it's not on btrfs, it's going to do nothing anyway. So I'm not sure conditional compilation is really warranted?

[ikerexxe]

Disregard my previous comment, it's based on a wrong assumption. I'm fine with the changes as they are, but as Alejandro already mentioned let's merge this after 4.19 is out

[Conan-Kudo]

Now that 4.19 is released, can we get this merged? @ikerexxe, it'd be great to get this backported into Fedora as well.

[alejandro-colomar]

Thanks for the reminder! I confirm my approval.

[alejandro-colomar]

Now that 4.19 is released, can we get this merged? @ikerexxe, it'd be great to get this backported into Fedora as well.

The next release will be in 2.5 months, so unless you really need to backport it, you could wait 2.5 months. Although it's up to Fedora (and @ikerexxe ) to decide, of course.

[Conan-Kudo]

Unfortunately, the next release will be after Fedora Linux 44 Final Freeze, so I'd rather have it backported now so we'd have it available for development in Fedora Linux 44.