Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Next] BTRFS custom hooks #381

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

[Next] BTRFS custom hooks #381

wants to merge 10 commits into from
+24 −2

Conversation

mwoz123
Copy link

@mwoz123 mwoz123 commented Jan 2, 2025 •
edited
Loading

Allows creation a custom hooks for btrfs filesystem,

Fixes #365 - allows using timeshift when, e.g., /boot is on a separate partition or other individual setup that requires tunning.

Tested on Ubuntu 24.04.

Locations:

  • backup scripts /etc/timeshift/backup-hooks.d/*
  • restore scripts /etc/timeshift/restore-hooks.d/*

Scripts filename requirements:

the names must consist entirely of ASCII upper- and lower-case letters, ASCII digits, ASCII underscores, and ASCII minus-hyphens.

Above means e.g. restore.sh is invalid filename as it contains . symbol (which mean file will be ignored).

There is export TS_SNAPSHOT_PATH which equals current snapshot path (for use in both Restore & Backup hooks).

@mwoz123 mwoz123 changed the title BTRFS custom hooks [WIP] BTRFS custom hooks Jan 2, 2025
@mwoz123 mwoz123 changed the title [WIP] BTRFS custom hooks BTRFS custom hooks Jan 2, 2025
@mwoz123 mwoz123 marked this pull request as ready for review January 2, 2025 17:27
ygerlach
ygerlach reviewed Jan 5, 2025
View reviewed changes
Copy link
Contributor

@ygerlach ygerlach left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess we should add Depends: debianutils (or just a Recommendsor Suggests ?) in the debian/control file.

And maybe a check for the existence of run-parts in main:

timeshift/src/Core/Main.vala

Line 355 in 07f5589

string[] dependencies = { "rsync","/sbin/blkid","df","mount","umount","fuser","crontab","cp","rm","touch","ln","sync","which"}; //"shutdown","chroot",

src/Core/Main.vala Outdated Show resolved Hide resolved
src/Core/Main.vala Outdated Show resolved Hide resolved
@clefebvre clefebvre changed the title BTRFS custom hooks [Next] BTRFS custom hooks Jan 6, 2025
@mwoz123
Copy link
Author

mwoz123 commented Jan 6, 2025

@ygerlach thanks for your review. I did changes per your request.

In addition I decided to add export TS_SNAPSHOT_PATH with path to current snapshot (for both backup & restore) - readme updated accordingly ;)

@mwoz123 mwoz123 requested a review from ygerlach January 7, 2025 15:28
@ygerlach
Copy link
Contributor

ygerlach commented Jan 7, 2025 •
edited
Loading

I am no maintainer - just another contributor - but i feel honoured by your review request :)

It looks good to me.

EDIT:
i just noticed: snapshot_to_restore.path may need to be escaped for ".

@mwoz123
Copy link
Author

mwoz123 commented Jan 7, 2025 •
edited
Loading

@ygerlach thanks :)

Hmmm, about

i just noticed: snapshot_to_restore.path may need to be escaped for ".

I don't quite understand. It seems to be escaped...

" export TS_SNAPSHOT_PATH=\"" + snapshot_to_restore.path + "\" &&" +

there's a \" between = and "; and another one \" between + and &&"

I also tested it and it seems to be working fine...
Could you help me understand what might be wrong/how I should change it?

@mwoz123 mwoz123 mentioned this pull request Jan 7, 2025
Open
@ygerlach
Copy link
Contributor

ygerlach commented Jan 8, 2025

I don't quite understand. It seems to be escaped...

Yes it is, but if the variable contains a " it may "unescape" itself. Because your code would generate something like this:

...
export TS_SNAPSHOT_PATH="/my/path/with"inside/" &&
...

It is very uncommon to have a " or ' inside a path, but it is allowed and generally possible.

and because it is "and not 'bash parameter expansion could also be possible:

...
export TS_SNAPSHOT_PATH="/my/path/with/${RANDOM}/inside/$(gnome-calculator)/" &&
...

But in this specific case it might be fine, because the path is somewhat fixed by the surrounding code of timeshift. It is not exactly user controlled input. So i am not sure if that would need fixing.

@mwoz123
Copy link
Author

mwoz123 commented Jan 9, 2025 •
edited
Loading

TS_SNAPSHOT_PATH = path to btrfs snapshot, which is static format and something like
/run/timeshift/9798/backup/timeshift-btrfs/snapshots/2024-09-26_19-42-58

It doesn't contain " nor '. But as I'm using ", only " (not ') in path could potentially have been an issue.

I tried to find a way to change/set a custom path, but I didn't find anything. Seems you've same observations:

It is not exactly user controlled input. So i am not sure if that would need fixing.

Exactly, agree.
Anyway, if someone still thinks - (having in Timeshfit) a user-defined btrfs snapshot backup path that contains " is possible - please let me know then I'll explore ways to change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ygerlach ygerlach ygerlach approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Broken system after restore (when /boot is on separate partition)
2 participants
@mwoz123 @ygerlach