From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] Improvement of backup iso script
Date: Mon, 02 Jan 2017 15:07:24 +0000
Message-ID: <1483369644.13949.331.camel@ipfire.org>
In-Reply-To: <1483369499.2414.6@mail01.ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============6598388602442192383=="
List-Id: <development.lists.ipfire.org>

--===============6598388602442192383==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

On Mon, 2017-01-02 at 16:04 +0100, Jonatan Schlag wrote:
> 
> Am Mo, 2. Jan, 2017 um 3:47 schrieb Michael Tremer 
> <michael.tremer(a)ipfire.org>:
> > 
> > Hi,
> > 
> > thanks for having a look at this :)
> > 
> > On Mon, 2017-01-02 at 15:36 +0100, Jonatan Schlag wrote:
> > > 
> > >  The backup iso script did not check the arch of the host. On x86_64 
> > > host
> > >  the wrong iso was downloaded.
> > > 
> > >  Furthermore, there were some if clauses which could cause trouble 
> > > which
> > >  I also tried to improve.
> > >  (For example: -e is valid if we have a directory or a file, but we 
> > > want
> > >  to check for a file only )
> > > 
> > >  Fixes: 11258
> > > 
> > >  Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> > >  ---
> > >   src/scripts/backupiso | 31 ++++++++++++++++++++++++++-----
> > >   1 file changed, 26 insertions(+), 5 deletions(-)
> > > 
> > >  diff --git a/src/scripts/backupiso b/src/scripts/backupiso
> > >  index 014e8e9..e92f385 100644
> > >  --- a/src/scripts/backupiso
> > >  +++ b/src/scripts/backupiso
> > >  @@ -1,9 +1,29 @@
> > >   #!/bin/sh
> > >  +arch=$(uname -m)
> > >  +
> > >  +case $arch in
> > >  +	"i586")
> > >  +	arch="i586"
> > >  +	echo "Your arch is $arch"
> > >  +	;;
> > >  +	"i686")
> > >  +	arch="i586"
> > >  +	echo "Your arch is $arch"
> > >  +	;;
> > >  +	"x86_64")
> > >  +	arch="x86_64"
> > >  +	echo "Your arch is $arch"
> > >  +	;;
> > >  +	*)
> > >  +	echo "Arch is not supported"
> > >  +	exit 1
> > >  +	;;
> > >  +esac
> > 
> > You could make this a bit shorter by using i?86 for i586 and i686. I 
> > also would
> > recommend to not use quotes for the matches.
> > 
> > I also indent it like this:
> > 
> > case x in
> >    a)
> >       some code
> >       ;;
> >    b)
> >       ...
> >       ;;
> > esac
> 
> I will send a new patch with better indention.
> > 
> > 
> > > 
> > >   COREVER=$(cat /opt/pakfire/db/core/mine)
> > >   # FIXME: edit this lines before release
> > >   
> > > URL="http://download.ipfire.org/releases/ipfire-2.x/2.19-core$COREVER/"
> > >  -ISO="ipfire-2.19.i586-full-core$COREVER.iso"
> > >  +ISO="ipfire-2.19.$arch-full-core$COREVER.iso"
> > > 
> > >   if [ -z $1 ]; then
> > >   	echo usage: $0 backup-file
> > >  @@ -13,9 +33,9 @@ fi
> > >   TS=$1
> > > 
> > >   mkdir -p /var/tmp/backupiso
> > >  -cd /var/tmp/backupiso
> > >  +(cd /var/tmp/backupiso || exit
> > > 
> Here 1
> 
> > 
> > > 
> > > 
> > >  -if [ ! -e ${ISO} ]
> > >  +if [ ! -f ${ISO} ]
> > >   then
> > >   	echo "Fetching ${URL}${ISO}"
> > >   	wget --quiet -c ${URL}${ISO}
> > >  @@ -26,7 +46,7 @@ wget --quiet -O ${ISO}.md5 ${URL}${ISO}.md5
> > > 
> > >   echo "Checking md5 of ${ISO}"
> > >   md5sum --status -c ${ISO}.md5
> > >  -if [ $? -eq 0 -o $? -eq 24 ]
> > >  +if [ $? -eq 0 ] || [ $? -eq 24 ]
> > >   then
> > >   	echo "md5 is OK"
> > >   else
> > 
> > It would be nice to know what 24 is for.
> 
> Yes, but I do not know it and after some search in the web I did not 
> found a explanation.
> I would keep the 24 till we can sure why it was added and If we could 
> remove it without problems.
> > 
> > 
> > 
> > > 
> > >  @@ -35,7 +55,7 @@ else
> > >   	wget --quiet -O ${ISO} ${URL}${ISO}
> > >   	echo "Checking again md5 of ${ISO}"
> > >   	md5sum --status -c ${ISO}.md5
> > >  -	if [ $? -eq 0 -o $? -eq 24 ]
> > >  +	if [ $? -eq 0 ] || [ $? -eq 24 ]
> > >   	then
> > >   		echo "md5 is OK"
> > >   	else
> > >  @@ -64,3 +84,4 @@ isohybrid $(basename ${ISO} .iso)-${TS}.iso
> > > 
> > >   echo "Cleaning up"
> > >   rm -rf backupiso.${TS}
> > >  +)
> > 
> > Why is this block executed in a sub-shell?
> 
> To fail gracefully (Here1) when /var/tmp/backupiso not exists.

That should never exist. Please use mktemp to create a temporary file.

-Michael

> 
> > 
> > 
> > 
> > 
> > -Michael
> 
> Jonatan
> 

--===============6598388602442192383==
Content-Type: application/pgp-signature
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="signature.asc"
MIME-Version: 1.0

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KVmVyc2lvbjogR251UEcgdjIKCmlRSWNCQUFC
Q2dBR0JRSllhbXl0QUFvSkVJQjU4UDl2a0FrSE9IWVAvUnJFMFdXdkd6enZvNWllTmJESGVWVFUK
TlQ2VldFd29tbHRSaS9KSlo3clh5Ry9RS25EWWl5RzZBdzZwRDBwSzVEZ09LUC96eXQzWXVnUjZu
NlA1QUE5egpNU2RZQlYxVkg4QXdXYlJheGZ0Zm1LaUFPaXlWQi8vN3hwNmZFUWJOTWlndlcrSXFE
VGExbmRBL0tKWHZEUkZRClAvdm9kSFNhNFhZNDMyTUp2M01Na0h0aWlRRUttQ2owbHEwcU5tazBV
bWZWNTl5QThIYzNJUzJuaWJwM0JhNUYKK2h4MFRPMWh4MEp1T0FMaVFzRVpuRkJkTTlHZnBSeGtL
aURsRWVPd0VyclQ3SkRzcytZNzZtVzNyaTM4YzlpdwpXbUdQK0YrOUdnVnI3N0tBMmlPOXBySnUx
SlovL1g4SldyeUVjTnZYVnhoNkN2dzl1VGN5Q1QzRTVnbnU5VHVqCkNIM3Q0Sm1YallNV1BuaW1p
NVQzTkdLdEQ3d3JQcDFuRW95WVBpcDNKKzZtQlpteWs1dnNvZHhaK0x1OHNCWXEKUU1QWTdlUVA4
RDFnRlZ2eURZRjcraHVqaHpCd0NYejhSdzdrQnh2RmEzWjYza0d2Q3JzaWcxSDVuUWFrZ1NObApm
SVd2LzJWdk9FS1k4bWRJbmJxTGU2ZWowVitEeGhYU0JObldGY3pZTjZ1YWY0K3YrVm0yWlpaSmZF
azBCWncyCm5BcmNNQWUvMWZrbmJmdXJVZlNMblYxVTNWV3RTak9kcHZDbUdjR1FBNWFXM2FDSkJD
QkxwdFAwYW1WY01SRnUKWVZ5T0o0b2RCa3QzeGQ5VWFmOWxnSzVXd0pxYnhPZXNFbHFXcEx4eXFL
NEV2azNhYW01eFFuTDYrODEraWY0dAovRkNrUHptczBPS0hZdjFzY2s5NAo9RTJKZgotLS0tLUVO
RCBQR1AgU0lHTkFUVVJFLS0tLS0K

--===============6598388602442192383==--