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

  From: Bernhard Bitsch <bbitsch@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
- 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@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 config/backup/backup.pl | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
  

Reviewed-by: Bernhard Bitsch <bbitsch@ipfire.org>
Tested-by: Bernhard Bitsch <bbitsch@ipfire.org>
Acked-by: Bernhard Bitsch <bbitsch@ipfire.org>

Am 25.03.2022 um 13:22 schrieb Adolf Belka:
> From: Bernhard Bitsch <bbitsch@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
> - 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@ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka@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() {
  

Hello Adolf,

> On 25 Mar 2022, at 12:22, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> From: Bernhard Bitsch <bbitsch@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?

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.

> - 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@ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka@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
>
  

Hi Michael,

On 28/03/2022 16:08, Michael Tremer wrote:
> Hello Adolf,
> 
>> On 25 Mar 2022, at 12:22, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>
>> From: Bernhard Bitsch <bbitsch@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.

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@ipfire.org>
>> Signed-off-by: Adolf Belka <adolf.belka@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
>>
>
  

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@ipfire.org> wrote:
>>>
>>> From: Bernhard Bitsch <bbitsch@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@ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka@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
>>>
>>
>
  

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@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@ipfire.org> wrote:
>>>> 
>>>> From: Bernhard Bitsch <bbitsch@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@ipfire.org>
>>>> Signed-off-by: Adolf Belka <adolf.belka@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
  

Hi Michael,

On 28/03/2022 18:40, Michael Tremer wrote:
> 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?
I am afraid that I can't. Running that ended up with the backup also trying to include all backup.ipf files into the backup. It even tried to backup the backup.ipf file from the backup being carried out and came up with the message that the file had changed while being backed up.

That happened when the pushd and popd were first used. Bernhard identified that the command was first outputting a whole list of files from the directory defined ie /. That is why it was changed to pushd / >/dev/null
> 
> 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 :)

I ran some bash testing. What I found is that in the above routine the file/path being read from the include file is not having the wildcards expanded so that the routine ends up, for instance with /var/ipfire/ddns/*.conf as the actual path/filename to check if it exists and of course it does not.

Regards,
Adolf.
> 
> -Michael
> 
>> On 28 Mar 2022, at 17:24, Adolf Belka <adolf.belka@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@ipfire.org> wrote:
>>>>>
>>>>> From: Bernhard Bitsch <bbitsch@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@ipfire.org>
>>>>> Signed-off-by: Adolf Belka <adolf.belka@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
>
  

Hi,

sorry for late reply.
I was busy with other things and suffering from my Covid infection.

Your discussion states the facts as found by me.

- the while loop reads the lines, exact file names and file name 
patterns ( wild cards )
- the for loop tries to expand the patterns to a list, expansion is done 
in the working directory.
- the existence check is of no use, if the pattern can't be expanded.
- the pushd / popd directives do a directory change based on a stack; 
the commands echo the actual stack to stdout, the redirection to 
/dev/null eleminates the inclusion in the file list ( which is written 
to stdout )
- pushd / changes to the root, where the patterns can be expanded, popd 
just returns to the old directory, where ever this was.

The error, Adolf found in the first version, can be explained as follows:
if the current directory was the backup dir, for inspection of the file 
generation, the stack ( output of pushd ) contains this directory. So 
the backup function tries to include all the files in the backup dir.

I've checked this behaviour by debug output ( the file list ). For the 
moment I think the function produces complete backups.

Remains the problem of incomplete archives so far.
The function itself may be written more efficient, but it is effective ( 
right) now.

Hope, I could explain sufficient the goal of the patch and the source of 
the error.

Regards,
Bernhard



Am 28.03.2022 um 20:22 schrieb Adolf Belka:
> Hi Michael,
> 
> On 28/03/2022 18:40, Michael Tremer wrote:
>> 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?
> I am afraid that I can't. Running that ended up with the backup also 
> trying to include all backup.ipf files into the backup. It even tried to 
> backup the backup.ipf file from the backup being carried out and came up 
> with the message that the file had changed while being backed up.
> 
> That happened when the pushd and popd were first used. Bernhard 
> identified that the command was first outputting a whole list of files 
> from the directory defined ie /. That is why it was changed to pushd / 
>  >/dev/null
>>
>> 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 :)
> 
> I ran some bash testing. What I found is that in the above routine the 
> file/path being read from the include file is not having the wildcards 
> expanded so that the routine ends up, for instance with 
> /var/ipfire/ddns/*.conf as the actual path/filename to check if it 
> exists and of course it does not.
> 
> Regards,
> Adolf.
>>
>> -Michael
>>
>>> On 28 Mar 2022, at 17:24, Adolf Belka <adolf.belka@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@ipfire.org> wrote:
>>>>>>
>>>>>> From: Bernhard Bitsch <bbitsch@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@ipfire.org>
>>>>>> Signed-off-by: Adolf Belka <adolf.belka@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
>>
  

Hello,

> On 28 Mar 2022, at 20:26, Bernhard Bitsch <bbitsch@ipfire.org> wrote:
> 
> Hi,
> 
> sorry for late reply.
> I was busy with other things and suffering from my Covid infection.

No worries. I hope you are better now.

> Your discussion states the facts as found by me.
> 
> - the while loop reads the lines, exact file names and file name patterns ( wild cards )
> - the for loop tries to expand the patterns to a list, expansion is done in the working directory.

Correct. This is where this breaks.

But prepending / to the string would change it back to what it was before my breaking change.

> - the existence check is of no use, if the pattern can't be expanded.

This is because when the shell cannot expand the globbing string, it will return it and we will try to include files which don’t exist. This could be changed by modifying the shell, but it looks like I went with this.

> - the pushd / popd directives do a directory change based on a stack; the commands echo the actual stack to stdout, the redirection to /dev/null eleminates the inclusion in the file list ( which is written to stdout )
> - pushd / changes to the root, where the patterns can be expanded, popd just returns to the old directory, where ever this was.

True, but changing the working directory isn’t the best way to solve this in my view.

> The error, Adolf found in the first version, can be explained as follows:
> if the current directory was the backup dir, for inspection of the file generation, the stack ( output of pushd ) contains this directory. So the backup function tries to include all the files in the backup dir.
> 
> I've checked this behaviour by debug output ( the file list ). For the moment I think the function produces complete backups.
> 
> Remains the problem of incomplete archives so far.
> The function itself may be written more efficient, but it is effective ( right) now.

Efficiency doesn’t really matter here. First of all this has to work. On my systems this function completes within not noticeable time.

-Michael

> Hope, I could explain sufficient the goal of the patch and the source of the error.
> 
> Regards,
> Bernhard
> 
> 
> 
> Am 28.03.2022 um 20:22 schrieb Adolf Belka:
>> Hi Michael,
>> On 28/03/2022 18:40, Michael Tremer wrote:
>>> 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?
>> I am afraid that I can't. Running that ended up with the backup also trying to include all backup.ipf files into the backup. It even tried to backup the backup.ipf file from the backup being carried out and came up with the message that the file had changed while being backed up.
>> That happened when the pushd and popd were first used. Bernhard identified that the command was first outputting a whole list of files from the directory defined ie /. That is why it was changed to pushd /  >/dev/null
>>> 
>>> 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 :)
>> I ran some bash testing. What I found is that in the above routine the file/path being read from the include file is not having the wildcards expanded so that the routine ends up, for instance with /var/ipfire/ddns/*.conf as the actual path/filename to check if it exists and of course it does not.
>> Regards,
>> Adolf.
>>> 
>>> -Michael
>>> 
>>>> On 28 Mar 2022, at 17:24, Adolf Belka <adolf.belka@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@ipfire.org> wrote:
>>>>>>> 
>>>>>>> From: Bernhard Bitsch <bbitsch@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@ipfire.org>
>>>>>>> Signed-off-by: Adolf Belka <adolf.belka@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
>>>