-
Notifications
You must be signed in to change notification settings - Fork 127
[Next] Configurable Btrfs subvol names #480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Save and load config for btrfs subvolume names. Added variables for home and root subvolumes to Main.
…e as system subvolume name
|
Thanks for the quick review, I'll get to the improvement suggestions soon :) Regarding:
What would "Disabled" do?
Thanks, definitely, that seems like it could fail spectacularly. |
the same as not selecting the checkbox seen in the screenshot. (I would replace the checkbox with the dropdown) |
|
I put the option into the backup type page, because it depends on the type of backup used. Selecting rsync is the equivalent of the option being disabled. To me, it would be a little confusing to have it on the users page, but I'm also not super familiar with the full functionality of this page, because I saw it allows selecting home directories in cases where multiple users exist on the system. Please let me know what you think. |
Also deduplicated the subvolume ui selection code
|
I've turned the subvolume name selection into a combo box, which should address almost all of the issues you had, except the one about the location of the option Not entirely sure what the common options are (please let me know) and if there should be a "custom" option as well. The custom option would make it tricky in terms of making sure the config is valid. |
into a single combobox to select a predefined layout
|
Actually, I went and change the two comboboxes into a single one, to select a predefined layout. Hopefully I left some room to add a "custom" option (implementation would be similiar how the custom date format is done, presumably). The current state would be more than sufficient for my - and probably most users - needs. Still not sure about the debian layout/subvolume names. Also, I'd like to change some of the messages in the UI, but I'm not sure how to proceed in regards to the translation, example:
|
|
@ygerlach apologies for the ping, but any advice or suggestions how to proceed from here? |
@fused0 no problem, the PR looks fine to me.
i think we will get issues if people miss some layout. so we can add it then, if we miss it here.
i think that is reasonable Just checked the ui again on debian. Here how it looks (for others interested in this PR):
I think it looks nice. Making it "disabled" when rsync is active is nice.
Just a thought: do we really need a selection? Can't timeshift find the correct layout itself? Because usually there should only be one valid layout possible and if the user fucks up the selection, timeshift behaves badly. So best keep the user out of this decision? And if a user has a layout that matches multiple layouts, with subvolumes like So if we have some detection what subvolumes are existing and choose the correct layout and maybe display the chosen selection like: "Using subvolume layout: Ubuntu ( Do all systems always have a seperate home subvolume? (EDIT: No, my debian test vm aparently only has When i try to create a snapshot i get: Maybe you should run some tests with your branch on some different systems/vms (debian, fedora, linuxmint, ...) P.S: im not a timeshift maintainer, just an contributor, so my opinion is not binding :) |
Thanks, but after your reply, there is more to consider.
The layout actually is auto detected for the intial config values, based on the distro name. I wondered the same, when I added it. I suppose one could parse the fstab to find what subovlumes (if any) root and home map to... Seems like a simple enough solution for the entire problem. It would catch all reasonable user-defined layouts.
Unfortunately, due to how the timeshift handles the settings and wizard dialogs - more or less immediately overwriting the config - there can only be two options: Having no option in the UI at all, or supporting "custom" layouts with settings in the UI. Otherwise, the edited config would be overwritten when you accidentally (or not) open the settings or wizard. I think I'll go for it and add the custom layout option.
Now I know why I couldn't find anything on the name of the home subvolume in Debian. :) I'm wondering if timeshift would work if you configure it to not backup the home subvolume and leave the home subvolume option blank or wiht a dummy value - or if supporting layouts without a separate home would imply more work. I would very much appreciate if you could give it a try.
Unfortunately I'm on a rather limited and flaky internet connection at the moment.
Seems I wrongfully assumed you to be a maintainer :) Thank you for your detailed reply and suggestions, much appreciated. I suppose it would be good to get a maintainer to weigh in on the issue. Thanks, |
empty home subvolume name and force include_btrfs_home_for_backup to false
|
@ygerlach Interestingly, timeshitft handles leaving the home subvolume name empty somewhat gravefully. I've made some more tweaks to make it work on debian without a separate home subvolume - it feels a bit hacky, but it works! Now I'm wondering if this solution is adequate, or if parsing fstab and adding a custom input group is even needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements configurable Btrfs subvolume names to support different Linux distribution layouts beyond Ubuntu's default @ and @home convention. The implementation adds a UI for selecting preset layouts (Ubuntu, Debian, Fedora) or configuring custom subvolume names, addressing issues #368, #405, and #266.
Key Changes:
- Added
root_subvolume_nameandhome_subvolume_nameproperties to Main.vala with distro-specific defaults - Created UI in SnapshotBackendBox.vala for subvolume layout selection with preset and custom options
- Replaced all hardcoded
"@"and"@home"references throughout the codebase with configurable names
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Core/Main.vala | Added subvolume name properties, distro detection logic, config persistence, and updated all references to use configurable names |
| src/Gtk/SnapshotBackendBox.vala | Implemented UI for subvolume layout selection with combo box and custom entry fields |
| src/Gtk/UsersBox.vala | Added logic to disable home subvolume backup checkbox when home_subvolume_name is empty |
| src/Gtk/SnapshotListBox.vala | Updated size calculation displays to use configurable subvolume names |
| src/Gtk/RestoreWindow.vala | Updated restore logic to check for configurable home subvolume name |
| src/Core/Subvolume.vala | Updated subvolume removal logic to use configurable names |
| src/Core/SnapshotRepo.vala | Updated mount path handling to use configurable subvolume names |
| src/Core/Snapshot.vala | Updated snapshot reading logic to use configurable subvolume names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| && sys_subvolumes.has_key(home_subvolume_name) | ||
| && (sys_subvolumes[home_subvolume_name].id == subvol_id)){ | ||
|
|
||
| subvol = sys_subvolumes["@home"]; | ||
| subvol = sys_subvolumes[home_subvolume_name]; |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the issue on lines 4186-4189, this code is vulnerable to null pointer exceptions when home_subvolume_name is empty. If home_subvolume_name is an empty string (as it is for Debian), attempting to access sys_subvolumes[home_subvolume_name].id could fail.
Add an empty string check:
else if ((sys_subvolumes.size > 0)
&& (home_subvolume_name != "")
&& sys_subvolumes.has_key(home_subvolume_name)
&& (sys_subvolumes[home_subvolume_name].id == subvol_id)){There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this follows, as it checks if sys_subvolumes has the key home_subvolume_name - sys_subvolumes itself is generated from fstab and crypttab, so doesn't depend on home_subvolume_name or root_subvolume_name.
|
|
||
| if (bak.subvolumes.has_key("@home")){ | ||
| size += bak.subvolumes["@home"].total_bytes; | ||
| if (bak.subvolumes.has_key(App.home_subvolume_name)){ |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null pointer exception when App.home_subvolume_name is empty. If home_subvolume_name is an empty string (as configured for Debian), bak.subvolumes.has_key("") could match unintended entries or bak.subvolumes[""] could cause issues.
Consider adding an empty string check:
if ((App.home_subvolume_name != "") && bak.subvolumes.has_key(App.home_subvolume_name)){
size += bak.subvolumes[App.home_subvolume_name].total_bytes;
}| if (bak.subvolumes.has_key(App.home_subvolume_name)){ | |
| if ((App.home_subvolume_name != "") && bak.subvolumes.has_key(App.home_subvolume_name)){ |
src/Gtk/SnapshotBackendBox.vala
Outdated
| Gtk.TreeIter strore_iter; | ||
| int active = -1; | ||
| for (int idx = 0; idx < possible_layouts.length[0]; idx++) { | ||
| list_store.append(out strore_iter); | ||
| list_store.set(strore_iter, |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in variable name: 'strore_iter' should be 'store_iter'.
| Gtk.TreeIter strore_iter; | |
| int active = -1; | |
| for (int idx = 0; idx < possible_layouts.length[0]; idx++) { | |
| list_store.append(out strore_iter); | |
| list_store.set(strore_iter, | |
| Gtk.TreeIter store_iter; | |
| int active = -1; | |
| for (int idx = 0; idx < possible_layouts.length[0]; idx++) { | |
| list_store.append(out store_iter); | |
| list_store.set(store_iter, |
| custom_home_subvol_entry.focus_out_event.connect((entry1, event1) => { | ||
| App.home_subvolume_name = custom_home_subvol_entry.text; | ||
|
|
||
| // If home subolume name is empty, do not backup home. |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: 'subolume' should be 'subvolume'.
| // If home subolume name is empty, do not backup home. | |
| // If home subvolume name is empty, do not backup home. |
| } | ||
| if (bak.subvolumes.has_key("@home")){ | ||
| size += bak.subvolumes["@home"].unshared_bytes; | ||
| if (bak.subvolumes.has_key(App.home_subvolume_name)){ |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as lines 438-440. When App.home_subvolume_name is empty (e.g., for Debian), accessing bak.subvolumes[""] could cause issues.
Add an empty string check:
if ((App.home_subvolume_name != "") && bak.subvolumes.has_key(App.home_subvolume_name)){
size += bak.subvolumes[App.home_subvolume_name].unshared_bytes;
}| if (bak.subvolumes.has_key(App.home_subvolume_name)){ | |
| if ((App.home_subvolume_name != "") && bak.subvolumes.has_key(App.home_subvolume_name)){ |
| } | ||
| else if (name == "@home"){ | ||
| path = path_combine(App.mount_point_app + "/backup-home", "@home"); | ||
| else if (name == App.home_subvolume_name){ |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential issue when App.home_subvolume_name is empty. For distributions like Debian that set home_subvolume_name = "", this condition name == "" will match any empty name, which could cause unintended behavior.
Consider adding an explicit check:
else if ((App.home_subvolume_name != "") && (name == App.home_subvolume_name)){| else if (name == App.home_subvolume_name){ | |
| else if ((App.home_subvolume_name != "") && (name == App.home_subvolume_name)){ |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
When root subvolume name is misconfigured to empty string, don't early out, but treat it the same as empty home subvolume name
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>



Here's the start of configurable subvolume names, to support non-ubuntu layout (at least layouts that only have different names, not nested ones).
It's functional, but I'm sure some (small?) issues are remaining.
Advice and suggestions appreciated.
See issues #368, #405, #266, etc
Best,
fused0