search

oetiker / znapzend

zfs backup with remote capabilities and mbuffer integration.
www.znapzend.org
GNU General Public License v3.0
609 stars 137 forks source link

Sanitise user inputs: Uppercase destination name results in an invalid ZFS preoprty name #476

Closed ShaunMaher closed 3 years ago

ShaunMaher commented 4 years ago

Hi.

Huge fan of ZnapZend.

I had an issue recently where I tried to run the following command:

sudo znapzendzetup edit \
>    SRC tank/lxd \
>    DST:MACH-HDD-002 '2w=>1d,4m=>1m,1y=>1y' MACH-HDD-002/lxd

The result was

*** backup plan: tank/lxd ***
dst_MACH-HDD-002 = MACH-HDD-002/lxd
dst_MACH-HDD-002_plan = 2weeks=>1day,10years=>1month
enabled         = on
mbuffer         = off
mbuffer_size    = 1G
post_znap_cmd   = off
pre_znap_cmd    = off
recursive       = on
src             = tank/lxd
src_plan        = 3days=>1hour,1week=>1day
tsformat        = %Y-%m-%d-%H%M%S

Do you want to save this backup set [y/N]? y
cannot set property for 'tank/lxd': invalid property 'org.znapzend:dst_MACH-HDD-002'
ERROR: could not set property dst_MACH-HDD-002 on tank/lxd

Obviously, this is because ZFS does not support uppercase characters in dataset property names. To save the next person some grief, could we please check the DST command line argument for invalid characters and either force the value to lowercase or fail, giving the user a descriptive error message.

Thank you kindly. Shaun.

jimklimov commented 4 years ago

Given that the user-provided tag X in dst_X is not related to actual pool names etc., lowercasing is not too good as it can lead to conflicts (what if you later try to define an MaCH-HdD-002?) that would be even weirder to resolve.

On the other hand, limits of one ZFS implementation might not apply to another (we are talking about nearly two decades of evolution across at least 3 completely unrelated kernels, and znapzend caters for all the best it can) so IMHO failing like it does now if your OS version does not support something is the way to go (after all, you as the user do get to see WHY it failed). It might not fail because it becomes supported on new pools in five years for now, and would be transparent to znapzend itself.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.