Re: [PATCH] backup.pl: fix Bug12817 - directories missing in backup after include file changed to relat>

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 4569 bytes --]

Hello Adolf,

So looking at this problem, it seems to be pretty bad. Any backup created with c164 is practically incomplete. Ouch.

So since c165 is already built and uploaded to the servers, I am not going to re-open it now.

I suppose fixing the bug ASAP is what we need to do for now before we are looking deeper into some long-term solution.

Could you confirm to me that this change is fixing the problem?

diff --git a/config/backup/backup.pl b/config/backup/backup.pl
index a2337cf23..a25c9a355 100644
--- a/config/backup/backup.pl
+++ b/config/backup/backup.pl
@@ -38,7 +38,7 @@ process_includes() {
        for include in $@; do
                local file
                while read -r file; do
-                       for file in ${file}; do
+                       for file in /${file}; do
                                if [ -e "/${file}" ]; then
                                        echo "${file}"
                                fi

It basically does the same as the pushd/popd set, but I find it a little bit more elegant :)

-Michael

> On 28 Mar 2022, at 17:24, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> Hi Michael,
> 
> On 28/03/2022 16:30, Adolf Belka wrote:
>> Hi Michael,
>> 
>> On 28/03/2022 16:08, Michael Tremer wrote:
>>> Hello Adolf,
>>> 
>>>> On 25 Mar 2022, at 12:22, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>>> 
>>>> From: Bernhard Bitsch <bbitsch(a)ipfire.org>
>>>> 
>>>> - The change of the backup include file from absolute paths to relative paths in CU164
>>>> commit https://git.ipfire.org/?p=ipfire-2.x.git;a=commit;h=c7e0d73e7cfd7be95db9d0a5f3392b8241813d5b
>>>> resulted in 20 core function directories no longer being backed up. Additionally a
>>>> similar effect occurred with some addon backups
>>> 
>>> Why is this happening? Is it because tar relied on the shell to expand the glowing characters?
>> I don't know. I found the issue, together with someone on the forum. Bernhard suggested the fix, which I tested, but I don't understand the issues behind the problem. Maybe Bernhard has more input on what might be causing the problem.
>>> 
>>> In that case, the file list should be changed that tar knows what files to pack and what not. I wouldn’t want to rely on globbing unless we have a reason that forces us.
>> Okay, will try and see if I can understand the basis for why the problem is occurring with the change of names from absolute to relative.
>> 
> I have tried to look at the code and see what the problem is. I can understand the essence of what the code is doing but not why it is going wrong, or at least yet.
> What a comparison of the files that are saved with the ones that are not saved shows that the ones not saved are those that would get selected by wildcards in the include file. All the ones saved look to be those with explicit filenames with no wild cards.
> 
> So something in either the process-includes subroutine or in the tar command is not working with wildcards.
> 
> I will try and see if I can set a small code loop up to see where the problem occurs.
> 
> Regards,
> Adolf.
>> Regards,
>> Adolf.
>>> 
>>>> - Fix applied here proposed by Bernhard Bitsch and tested on a vm testbed system and
>>>> confirmed to fix the problem. Backup of all directories again being done.
>>>> 
>>>> Fixes: Bug12817
>>>> Tested-By: Adolf Belka <adolf.belka(a)ipfire.org>
>>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>> ---
>>>> config/backup/backup.pl | 22 ++++++++++++----------
>>>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>>> 
>>>> diff --git a/config/backup/backup.pl b/config/backup/backup.pl
>>>> index a2337cf23..c7dbc6cae 100644
>>>> --- a/config/backup/backup.pl
>>>> +++ b/config/backup/backup.pl
>>>> @@ -35,16 +35,18 @@ list_addons() {
>>>> process_includes() {
>>>> local include
>>>> 
>>>> -  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
>>>> +  pushd / >/dev/null
>>>> +  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 >/dev/null
>>>> }
>>>> 
>>>> make_backup() {
>>>> -- 
>>>> 2.35.1
>>>> 
>>> 
>> 
> 
> -- 
> Sent from my laptop