omv-backup-vm script only deletes old backup files but doesn't remove them from /etc/omv-backup-vm.list

[DonkeeeyKong]

When called directly with the "-k" flag the /usr/sbin/omv-backup-vm script adds the new backup correctly to /etc/omv-backup-vm.list and correctly removes old backup files and folders as set with "-k" but it doesn't remove the deleted backups from the list causing them to remain visible in the web GUI.

[DonkeeeyKong]

Found the problem:

openmediavault-kvm/usr/sbin/omv-backup-vm

Line 272 in c17608f

delDir="$(dirname "${keepDir}")/"

This adds a "/" to the delDir variable. I have been running the script without a trailing slash when specifying the backup directory, so sed doesn't match a pattern here:

openmediavault-kvm/usr/sbin/omv-backup-vm

Line 277 in c17608f

sed -i "\|${delDir},${backupVm},${delDate}|d" ${listFile}

[DonkeeeyKong]

This PR (#24) should close this issue.

[ryecoaaron]

I'm confused by this fix. dirname will always return the parent directory without a trailing slash of the keepDir even if keepDir ends in slashes or not. Then a trailing slash is added to a value that couldn't have an ending slash. How would the log file have an entry without a trailing slash that sed fails to find?

[DonkeeeyKong]

You are right. The problem starts earlier though. With the variable backupDir, not with keepDir

Sed always looks for a pattern containing a trailing slash when deleting old entries.

I have been running the script with -d "/path/to/backup". Without trailing slash. That makes backupDir /path/to/backup. Also without trailing slash. That is saved in the list file without changes:

openmediavault-kvm/usr/sbin/omv-backup-vm

Line 254 in c17608f

echo "$(uuid),${backupDir},${backupVm},${date},${used}" >> ${listFile}

And because sed looks for delDir that always contains a trailing slash, in my case it has never found a matching pattern and my list file was full of deprecated entries.

My PR adds a trailing slash to backupDir if there is none and also checks for deprecated entries and removes them.

Correct me if I am wrong or if I misunderstood anything. After changing the script in my installation it has been working as intended though.

[ryecoaaron]

I didn't look at the whole PR. It was a lot of changes for something simple and should be a separate PR for the verifying part (I would rather that be in a separate script). Why not just improve the sed to something like this?

sed -i "|${delDir%/}/?,${backupVm},${delDate}|d" "${listFile}"

[DonkeeeyKong]

Why not just improve the sed to something like this?

sed -i "|${delDir%/}/?,${backupVm},${delDate}|d" "${listFile}"

Yes. That's probably better than my backupDir=$(echo "$backupDir" | sed "s|[^/]$|&/|"). Your approach works also if there are older entries without the slash which mine wouldn't.

I didn't look at the whole PR. It was a lot of changes for something simple and should be a separate PR for the verifying part (I would rather that be in a separate script).

No problem. The change for the slash was just one line. The biggest part is the script that looks for deprecated entries and deletes them. I didn't dare to propose a separate script. Of course it's not necessary to run that part every time. Doing it once after including the new sed pattern is probably enough to get rid of deprecated entries. I'm not experienced enough to implement that in a good way though.

[ryecoaaron]

Ok, I will push the sed change and work on a separate script that verifies that the backups for the logged entries actually exists and removes them if they don't exist.

c474665

[DonkeeeyKong]

Thanks a lot for all your work and your help!

Just one addition, in case it's not intentional: Would it not be nicer to also adjust the grep command that preceeds the now changed sed command? The result is the same but it would be more uniform.

[ryecoaaron]

Oops, I forgot that. Thanks for pointing it out.

8fca495