From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH] backup.pl: fix Bug12817 - directories missing in backup after include file changed to relat> Date: Tue, 29 Mar 2022 11:23:41 +0100 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1109833568345690351==" List-Id: --===============1109833568345690351== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 28 Mar 2022, at 20:26, Bernhard Bitsch wrote: >=20 > Hi, >=20 > 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. >=20 > - 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 ret= urn it and we will try to include files which don=E2=80=99t 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 c= ommands echo the actual stack to stdout, the redirection to /dev/null elemina= tes the inclusion in the file list ( which is written to stdout ) > - pushd / changes to the root, where the patterns can be expanded, popd jus= t returns to the old directory, where ever this was. True, but changing the working directory isn=E2=80=99t 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 gen= eration, the stack ( output of pushd ) contains this directory. So the backup= function tries to include all the files in the backup dir. >=20 > I've checked this behaviour by debug output ( the file list ). For the mome= nt I think the function produces complete backups. >=20 > Remains the problem of incomplete archives so far. > The function itself may be written more efficient, but it is effective ( ri= ght) now. Efficiency doesn=E2=80=99t 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 th= e error. >=20 > Regards, > Bernhard >=20 >=20 >=20 > Am 28.03.2022 um 20:22 schrieb Adolf Belka: >> Hi Michael, >> On 28/03/2022 18:40, Michael Tremer wrote: >>> Hello Adolf, >>>=20 >>> So looking at this problem, it seems to be pretty bad. Any backup created= with c164 is practically incomplete. Ouch. >>>=20 >>> So since c165 is already built and uploaded to the servers, I am not goin= g to re-open it now. >>>=20 >>> I suppose fixing the bug ASAP is what we need to do for now before we are= looking deeper into some long-term solution. >>>=20 >>> 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 tryin= g to include all backup.ipf files into the backup. It even tried to backup th= e backup.ipf file from the backup being carried out and came up with the mess= age 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 directo= ry defined ie /. That is why it was changed to pushd / >/dev/null >>>=20 >>> 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 >>>=20 >>> 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 fil= e/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. >>>=20 >>> -Michael >>>=20 >>>> On 28 Mar 2022, at 17:24, Adolf Belka wrote: >>>>=20 >>>> Hi Michael, >>>>=20 >>>> On 28/03/2022 16:30, Adolf Belka wrote: >>>>> Hi Michael, >>>>>=20 >>>>> On 28/03/2022 16:08, Michael Tremer wrote: >>>>>> Hello Adolf, >>>>>>=20 >>>>>>> On 25 Mar 2022, at 12:22, Adolf Belka wrot= e: >>>>>>>=20 >>>>>>> From: Bernhard Bitsch >>>>>>>=20 >>>>>>> - The change of the backup include file from absolute paths to relati= ve paths in CU164 >>>>>>> commit https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommit;h=3Dc7e0= d73e7cfd7be95db9d0a5f3392b8241813d5b=20 >>>>>>> resulted in 20 core function directories no longer being backed up. A= dditionally a >>>>>>> similar effect occurred with some addon backups >>>>>>=20 >>>>>> 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. Be= rnhard suggested the fix, which I tested, but I don't understand the issues b= ehind the problem. Maybe Bernhard has more input on what might be causing the= problem. >>>>>>=20 >>>>>> In that case, the file list should be changed that tar knows what file= s to pack and what not. I wouldn=E2=80=99t 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 proble= m is occurring with the change of names from absolute to relative. >>>>>=20 >>>> I have tried to look at the code and see what the problem is. I can unde= rstand the essence of what the code is doing but not why it is going wrong, o= r 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 wil= dcards in the include file. All the ones saved look to be those with explicit= filenames with no wild cards. >>>>=20 >>>> So something in either the process-includes subroutine or in the tar com= mand is not working with wildcards. >>>>=20 >>>> I will try and see if I can set a small code loop up to see where the pr= oblem occurs. >>>>=20 >>>> Regards, >>>> Adolf. >>>>> Regards, >>>>> Adolf. >>>>>>=20 >>>>>>> - Fix applied here proposed by Bernhard Bitsch and tested on a vm tes= tbed system and >>>>>>> confirmed to fix the problem. Backup of all directories again being d= one. >>>>>>>=20 >>>>>>> Fixes: Bug12817 >>>>>>> Tested-By: Adolf Belka >>>>>>> Signed-off-by: Adolf Belka >>>>>>> --- >>>>>>> config/backup/backup.pl | 22 ++++++++++++---------- >>>>>>> 1 file changed, 12 insertions(+), 10 deletions(-) >>>>>>>=20 >>>>>>> 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 >>>>>>>=20 >>>>>>> - 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 >>>>>>> } >>>>>>>=20 >>>>>>> make_backup() { >>>>>>> --=20 >>>>>>> 2.35.1 >>>>>>>=20 >>>>>>=20 >>>>>=20 >>>>=20 >>>> --=20 >>>> Sent from my laptop >>>=20 --===============1109833568345690351==--