Bug 12817

Summary: Many directories missing from backup after include file changed to relative paths
Product: IPFire Reporter: Adolf Belka <adolf.belka>
Component: ---Assignee: Adolf Belka <adolf.belka>
Status: CLOSED FIXED QA Contact:
Severity: Major Usability    
Priority: Will affect all users CC: bbitsch, michael.tremer, rejeancgrpq
Version: 2   
Hardware: unspecified   
OS: Unspecified   
See Also: https://bugzilla.ipfire.org/show_bug.cgi?id=12816
Attachments: Backup from CU163
Backup from CU164
Backup from CU164 after leading / added back in to include file

Description Adolf Belka 2022-03-24 13:33:36 UTC
After the include/exclude files had their entries changed to relative paths many directories in /var/ipfire are no longer being backed up. This has come in with Core Update 164.

The change involved is 
https://git.ipfire.org/?p=ipfire-2.x.git;a=commit;h=c7e0d73e7cfd7be95db9d0a5f3392b8241813d5b

The directories missing are

    captive
    connscheduler
    ddns
    dhcp
    dhcpc
    dma
    accounting addon
    dnsforward
    extrahd
    isdn
    logging
    mac
    main
    modem
    optionsfw
    pakfire
    qos
    remote
    sensors
    wakeonlan
    wio addon
    wireless

There may be more depending on the addons installed. In my case two of my addons no longer are backed up.

At first I thought this might be related to the paths issue with backupctrl
https://bugzilla.ipfire.org/show_bug.cgi?id=12811
That fix has been merged into next so I installed the latest nightly from next and tested it out. It looks like the backupctrl issue may be fixed but the same directories were missing still.

I added the leading / to every entry in the /var/ipfire/backup/include file and the backup was including the directories again.

Unless people try to restore from a backup they won't notice this, although the backup sizes are smaller than before because of the missing files.

This was flagged up in the forum due to missing ddns files after doing a restore.
https://community.ipfire.org/t/cu-163-164-165-rc-backup-restore-dyndns-entry-missing/7469

I was able to reproduce the same effect on my systems and making the paths in the include file absolute again made the missing directories be backed up again
Comment 1 Adolf Belka 2022-03-24 13:34:39 UTC
Created attachment 1022 [details]
Backup from CU163
Comment 2 Adolf Belka 2022-03-24 13:35:05 UTC
Created attachment 1023 [details]
Backup from CU164
Comment 3 Adolf Belka 2022-03-24 13:35:49 UTC
Created attachment 1024 [details]
Backup from CU164 after leading / added back in to include file
Comment 4 Rejjy_S 2022-03-24 13:57:57 UTC
I had submitted bug 12816 earlier which may be related.
Comment 5 Bernhard Bitsch 2022-03-24 14:32:36 UTC
Function 'process_includes' in backup.pl should do a

pushd /

at begin and a 

popd

at end.

This reflects the invocation of tar

tar cvfz "${filename}" -C / \
		--exclude-from="/var/ipfire/backup/exclude" \
		--exclude-from="/var/ipfire/backup/exclude.user" \
		$(process_includes "/var/ipfire/backup/include") \
		$(process_includes "/var/ipfire/backup/include.user") \
		"$@"


The tar command isn't completely symmetric, --exclude-from accepts pattern in the file, the correspondig -T option accepts filenames only.
Comment 6 Adolf Belka 2022-03-24 14:55:19 UTC
The pushd and popd you mention are bash commands correct?

I was able to find pushd available as a perl module but not popd but both pushd and popd as bash commands.

So the commands would need to be run as system calls in perl but I think the devs prefer to avoid that as much as possible as system calls have some security weakness.

I will compare the bash and perl pushd commands and see what perl uses at the end of running a pushd
Comment 7 Bernhard Bitsch 2022-03-24 15:00:00 UTC
Hi,

Am 24.03.2022 um 15:55 schrieb IPFire Bugzilla:
> *Comment # 6 <https://bugzilla.ipfire.org/show_bug.cgi?id=12817#c6> on 
> bug 12817 <https://bugzilla.ipfire.org/show_bug.cgi?id=12817> from Adolf 
> Belka <mailto:adolf.belka@ipfire.org> *
> 
> The pushd and popd you mention are bash commands correct?
> 

Correct!

> I was able to find pushd available as a perl module but not popd but both pushd
> and popd as bash commands.
> > So the commands would need to be run as system calls in perl but I 
think the
> devs prefer to avoid that as much as possible as system calls have some
> security weakness.
> 
> I will compare the bash and perl pushd commands and see what perl uses at the
> end of running a pushd
> 

Not necessary. ;) Despite the name 'backup.pl' is a bash script. So the 
commands can be used.

Regards,
Bernhard

> ------------------------------------------------------------------------
> You are receiving this mail because:
> 
>   * You are on the CC list for the bug.
>
Comment 8 Adolf Belka 2022-03-24 15:02:39 UTC
Thanks Bernhard. I had just seen the .pl and made the assumption (which is never good) that it was a perl file and didn't look closer at the content.

I will give it a test in my vm testbed system and report back how it goes.
Comment 9 Adolf Belka 2022-03-24 15:15:30 UTC
It didn't work as it ended up also trying to add all the existing *.ipf backup files in /var/ipfire/backup and it should be ignoring them.

Just to check this is what I changed the function in backup.pl to. Maybe I did it incorrectly, not fully understanding this yet.

process_includes() {
        local include
    pushd /
        for include in $@; do
                local file
                while read -r file; do
                        for file in ${file}; do
                                if [ -e "/${file}" ]; then
                                        echo "${file}"
                                fi
                        done
                done < "${include}"
        done | sort -u
    popd
}
Comment 10 Bernhard Bitsch 2022-03-24 15:57:25 UTC
Am 24.03.2022 um 16:15 schrieb IPFire Bugzilla:
> *Comment # 9 <https://bugzilla.ipfire.org/show_bug.cgi?id=12817#c9> on 
> bug 12817 <https://bugzilla.ipfire.org/show_bug.cgi?id=12817> from Adolf 
> Belka <mailto:adolf.belka@ipfire.org> *
> 
> It didn't work as it ended up also trying to add all the existing *.ipf backup
> files in /var/ipfire/backup and it should be ignoring them.
> 
> Just to check this is what I changed the function in backup.pl to. Maybe I did
> it incorrectly, not fully understanding this yet.
> 
> process_includes() {
>          local include
>      pushd /
>          for include in $@; do
>                  local file
>                  while read -r file; do
>                          for file in ${file}; do
>                                  if [ -e "/${file}" ]; then
>                                          echo "${file}"
>                                  fi
>                          done
>                  done < "${include}"
>          done | sort -u
>      popd
> }
> 

This implemented right. In investigation of your report I found that 
pushd and popd output the dir stack. This shouldn't go to the file list, 
so the exact invocation should be
pushd / >/dev/null
popd >/dev/null

I've tried the modification with execution of
/var/ipfire/backup.pl list
There are no '.ipf files in the included file list.
Comment 11 Adolf Belka 2022-03-24 16:51:00 UTC
(In reply to Bernhard Bitsch from comment #10)

> so the exact invocation should be
> pushd / >/dev/null
> popd >/dev/null
> 

I tried that in my vm testbed and the backup worked as expected.

Thanks very much Bernhard.

Will you raise a patch for this or should I do it on your behalf?
Comment 12 Bernhard Bitsch 2022-03-24 16:56:36 UTC
Hi Adolf,

Am 24.03.2022 um 17:51 schrieb IPFire Bugzilla:
> *Comment # 11 <https://bugzilla.ipfire.org/show_bug.cgi?id=12817#c11> on 
> bug 12817 <https://bugzilla.ipfire.org/show_bug.cgi?id=12817> from Adolf 
> Belka <mailto:adolf.belka@ipfire.org> *
> 
> (In reply to Bernhard Bitsch fromcomment #10  <show_bug.cgi?id=12817#c10>)
> 
>> so the exact invocation should be  > pushd / >/dev/null > popd >/dev/null >
> 
> I tried that in my vm testbed and the backup worked as expected.
> 

Maybe you were in the backup directory. Thus the commands report this 
and the dir is included.

> Thanks very much Bernhard.
> 
> Will you raise a patch for this or should I do it on your behalf?
> 
I think it's just easier, if you do that. At the moment I don't have an 
environment for sending patches.
Should do it. ;)

Thx,
Bernhard
Comment 13 Bernhard Bitsch 2022-03-25 09:44:32 UTC
BTW:
is there a possibility to introduce this change into the running systems?
Either by replacing backup.pl or changing all entries in include and include.user to absolute paths.

This would make the 'brute-force repair method' backup-reinstall-restore functioning again.
Comment 14 Adolf Belka 2022-03-25 11:53:18 UTC
(In reply to Bernhard Bitsch from comment #13)
> BTW:
> is there a possibility to introduce this change into the running systems?
> Either by replacing backup.pl or changing all entries in include and
> include.user to absolute paths.
> 
> This would make the 'brute-force repair method' backup-reinstall-restore
> functioning again.

This can be done but only manually by each individual as far as I can tell.

I had changed the include file back to absolute paths on my running system but now I have moved include back to the relative paths and added the pushd/popd fix into my backup.pl file and that is running well.

You may have to redo that after any Core Update until the Core Update that ends up with the patch fix on it. I suspect this will be CU166
Comment 15 Adolf Belka 2022-03-25 12:25:50 UTC
Patch to fix this bug based on Bernhard Bitsch input has been submitted to the Dev Mailing list and patchwork.

https://patchwork.ipfire.org/project/ipfire/patch/20220325122247.7036-1-adolf.belka@ipfire.org/
Comment 16 Adolf Belka 2022-03-30 16:53:49 UTC
@Michael has submitted an updated patch.

https://patchwork.ipfire.org/project/ipfire/patch/20220329122711.558230-1-michael.tremer@ipfire.org/
Comment 17 Adolf Belka 2022-04-01 13:36:52 UTC
Patch has been released in CU166

https://www.ipfire.org/download/ipfire-2.27-core166

Fix confirmed on production machine and two vm's.
Comment 18 Adolf Belka 2022-04-01 20:38:42 UTC
*** Bug 12816 has been marked as a duplicate of this bug. ***