Do not exit from handler
As note at #3692 (closed), running exit in the rsync handler is not correct as it's code is sourced by the main backupninja program.
(from redmine: created on 2011-12-31, closed on 2012-05-13)
Activity
rhatto wrote:
As note at #3692 (closed), running exit in the rsync handler is not correct as it's code is sourced by the main backupninja program.
Commit "ab797ebace185b062ab970b8b4acb8150ed026ab":https://git.sarava.org/?p=backupninja.git;a=commit;h=ab797ebace185b062ab970b8b4acb8150ed026ab removing most of 'exit' commands, except:
- Those running at the remote destination.
- Those with zero status code, as I don't know how to handle these using backupninja functions. Those are not intended to generate warning messages.
(from redmine: written on 2011-12-31)
rhatto wrote:
rhatto wrote:
As note at #3692 (closed), running exit in the rsync handler is not correct as it's code is sourced by the main backupninja program.
Commit "ab797ebace185b062ab970b8b4acb8150ed026ab":https://git.sarava.org/?p=backupninja.git;a=commit;h=ab797ebace185b062ab970b8b4acb8150ed026ab removing most of 'exit' commands,
Nice. Cherry-picked.
except:
- Those running at the remote destination.
Probably right, I guess.
- Those with zero status code, as I don't know how to handle these using backupninja functions. Those are not intended to generate warning messages.
Maybe we should create a new function, alongside fatal / warning etc., that would be called @success@?
(from redmine: written on 2012-01-01)
Trying to have this fixed to release 1.0 soon.
There are only two remaining calls to exit (apart of those run on the remote host):
In the @check_lockfile@ function: it seems redundant to me, and this should quickly disappear now that backupninja itself has got locking support; I'm going to open a dedicated ticket about it; let's ignore it as far as the current ticket is concerned.
In the @rotate_long@ function, one may read:
if [ ! -d "$backuproot" ]; then echo "Debug: skipping rotate of $backuproot as it doesn't exist." exit fiIt seems to me the @rotate_long@ function, called for every include, is not the correct place to exit and skip the whole rsync backup job entirely (that is, skip processing of other includes too), but I don't know this code well and only had a quick look.
Anyway, it seems to me that: either the debug message (skipping rotate of $backuproot as it doesn't exist) is correct, and the @exit@ should be turned into a mere @return@; or the debug message is incorrect, and this error condition is really worth exiting the whole rsync backup job, and then @exit@ should be turned into a @fatal@ (generally, I don't think we want to abort silently if backups cannot be done as configured).
rhatto, did I miss something?
(from redmine: written on 2012-02-25)
intrigeri wrote:
In the @check_lockfile@ function: it seems redundant to me, and this should quickly disappear now that backupninja itself has got locking support; I'm going to open a dedicated ticket about it; let's ignore it as far as the current ticket is concerned.
Ok.
In the @rotate_long@ function, one may read:
[...]
It seems to me the @rotate_long@ function, called for every include, is not the correct place to exit and skip the whole rsync backup job entirely (that is, skip processing of other includes too), but I don't know this code well and only had a quick look.
You're right.
Anyway, it seems to me that: either the debug message (skipping rotate of $backuproot as it doesn't exist) is correct, and the @exit@ should be turned into a mere @return@; or the debug message is incorrect, and this error condition is really worth exiting the whole rsync backup job, and then @exit@ should be turned into a @fatal@ (generally, I don't think we want to abort silently if backups cannot be done as configured).
rhatto, did I miss something?
You didn't missed anything. You just found another @exit@ that should be changed by a @fatal@. So I commited two changes:
- "bf5ada0f":https://git.sarava.org/?p=backupninja.git;a=commit;h=bf5ada0f0357dd3b7500d03dd4a4a2f84282e8c1 changes @exit@ to @fatal@.
- "ba3e3759":https://git.sarava.org/?p=backupninja.git;a=commit;h=ba3e37595d7a8fbf9a881ab0b9b14146eb91f3a2 fixes the overall function behavior, i.e, prevents that the handler exit without processing the other includes.
Both are on my "bugs/3721":https://git.sarava.org/?p=backupninja.git;a=shortlog;h=refs/heads/bug/3721 branch.
Please note that I cannot do the same in the functions intended to run in the remote side. Those still contain @exit@ statements.
(from redmine: written on 2012-05-10)
Merged, thanks.
(from redmine: written on 2012-05-13)
- micah Status changed to closed
Status changed to closed