From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernhard Bitsch To: development@lists.ipfire.org Subject: Re: [PATCH] backup.pl: fix Bug12817 - directories missing in backup after include file changed to relat> Date: Mon, 28 Mar 2022 21:26:51 +0200 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3109926809716699559==" List-Id: --===============3109926809716699559== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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=20 patterns ( wild cards ) - the for loop tries to expand the patterns to a list, expansion is done=20 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;=20 the commands echo the actual stack to stdout, the redirection to=20 /dev/null eleminates the inclusion in the file list ( which is written=20 to stdout ) - pushd / changes to the root, where the patterns can be expanded, popd=20 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=20 generation, the stack ( output of pushd ) contains this directory. So=20 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=20 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 (=20 right) now. Hope, I could explain sufficient the goal of the patch and the source of=20 the error. Regards, Bernhard Am 28.03.2022 um 20:22 schrieb Adolf Belka: > Hi Michael, >=20 > On 28/03/2022 18:40, Michael Tremer wrote: >> Hello Adolf, >> >> So looking at this problem, it seems to be pretty bad. Any backup=20 >> created with c164 is practically incomplete. Ouch. >> >> So since c165 is already built and uploaded to the servers, I am not=20 >> going to re-open it now. >> >> I suppose fixing the bug ASAP is what we need to do for now before we=20 >> 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=20 > trying to include all backup.ipf files into the backup. It even tried to=20 > backup the backup.ipf file from the backup being carried out and came up=20 > with the message that the file had changed while being backed up. >=20 > That happened when the pushd and popd were first used. Bernhard=20 > identified that the command was first outputting a whole list of files=20 > from the directory defined ie /. That is why it was changed to pushd /=20 > >/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() { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for include in $@; do >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 local file >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 while read -r file; do >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for file in ${fi= le}; do >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for file in /${f= ile}; do >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if [ -e "/${file}" ]; then >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 echo "${file}" >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fi >> >> It basically does the same as the pushd/popd set, but I find it a=20 >> little bit more elegant :) >=20 > I ran some bash testing. What I found is that in the above routine the=20 > file/path being read from the include file is not having the wildcards=20 > expanded so that the routine ends up, for instance with=20 > /var/ipfire/ddns/*.conf as the actual path/filename to check if it=20 > exists and of course it does not. >=20 > Regards, > Adolf. >> >> -Michael >> >>> On 28 Mar 2022, at 17:24, Adolf Belka 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 wrote: >>>>>> >>>>>> From: Bernhard Bitsch >>>>>> >>>>>> - The change of the backup include file from absolute paths to=20 >>>>>> relative paths in CU164 >>>>>> commit=20 >>>>>> https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommit;h=3Dc7e0d73e7cfd= 7be95db9d0a5f3392b8241813d5b=20 >>>>>> >>>>>> resulted in 20 core function directories no longer being backed=20 >>>>>> up. Additionally a >>>>>> similar effect occurred with some addon backups >>>>> >>>>> Why is this happening? Is it because tar relied on the shell to=20 >>>>> expand the glowing characters? >>>> I don't know. I found the issue, together with someone on the forum.=20 >>>> Bernhard suggested the fix, which I tested, but I don't understand=20 >>>> the issues behind the problem. Maybe Bernhard has more input on what=20 >>>> might be causing the problem. >>>>> >>>>> In that case, the file list should be changed that tar knows what=20 >>>>> files to pack and what not. I wouldn=E2=80=99t want to rely on globbing= =20 >>>>> unless we have a reason that forces us. >>>> Okay, will try and see if I can understand the basis for why the=20 >>>> problem is occurring with the change of names from absolute to=20 >>>> relative. >>>> >>> I have tried to look at the code and see what the problem is. I can=20 >>> understand the essence of what the code is doing but not why it is=20 >>> going wrong, or at least yet. >>> What a comparison of the files that are saved with the ones that are=20 >>> not saved shows that the ones not saved are those that would get=20 >>> selected by wildcards in the include file. All the ones saved look to=20 >>> be those with explicit filenames with no wild cards. >>> >>> So something in either the process-includes subroutine or in the tar=20 >>> command is not working with wildcards. >>> >>> I will try and see if I can set a small code loop up to see where the=20 >>> problem occurs. >>> >>> Regards, >>> Adolf. >>>> Regards, >>>> Adolf. >>>>> >>>>>> - Fix applied here proposed by Bernhard Bitsch and tested on a vm=20 >>>>>> testbed system and >>>>>> confirmed to fix the problem. Backup of all directories again=20 >>>>>> being done. >>>>>> >>>>>> Fixes: Bug12817 >>>>>> Tested-By: Adolf Belka >>>>>> Signed-off-by: Adolf Belka >>>>>> --- >>>>>> 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 >>>>>> >>>>>> -=C2=A0 for include in $@; do >>>>>> -=C2=A0 local file >>>>>> -=C2=A0 while read -r file; do >>>>>> -=C2=A0 for file in ${file}; do >>>>>> -=C2=A0 if [ -e "/${file}" ]; then >>>>>> -=C2=A0 echo "${file}" >>>>>> -=C2=A0 fi >>>>>> -=C2=A0 done >>>>>> -=C2=A0 done < "${include}" >>>>>> -=C2=A0 done | sort -u >>>>>> +=C2=A0 pushd / >/dev/null >>>>>> +=C2=A0 for include in $@; do >>>>>> +=C2=A0 local file >>>>>> +=C2=A0 while read -r file; do >>>>>> +=C2=A0 for file in ${file}; do >>>>>> +=C2=A0 if [ -e "/${file}" ]; then >>>>>> +=C2=A0 echo "${file}" >>>>>> +=C2=A0 fi >>>>>> +=C2=A0 done >>>>>> +=C2=A0 done < "${include}" >>>>>> +=C2=A0 done | sort -u >>>>>> +=C2=A0 popd >/dev/null >>>>>> } >>>>>> >>>>>> make_backup() { >>>>>> --=20 >>>>>> 2.35.1 >>>>>> >>>>> >>>> >>> >>> --=20 >>> Sent from my laptop >> --===============3109926809716699559==--