From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: network@lists.ipfire.org
Subject: Re: [PATCH v3 2/5] util: add new function copy
Date: Mon, 17 Jul 2017 16:36:09 -0400
Message-ID: <1500323769.2548.16.camel@ipfire.org>
In-Reply-To: <1500318318-18852-2-git-send-email-jonatan.schlag@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============2803310128360250959=="
List-Id: <network.lists.ipfire.org>

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

Hi,

typos, typos, typos. Please go through the comments again. It really is
hard to read.

On Mon, 2017-07-17 at 21:05 +0200, Jonatan Schlag wrote:
> Just one short note, I could also insted of removing the destintaion
> if she exists,
> allow fwrite to overired files. I did not do this because it would
> first maybe break code
> when the function fwrite is used. Second it is easier to remove the
> destintaion and
> so allow fwrite writing the content to a plain file, then changing
> the function so that fwrite override files.
> It is easy to remove a file bevore but appending to a file with a
> function that overried files is not possible.
> So I think it is the best to keep this feature of fwrite and when
> somebody wants to override a file he has to remove it before.
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/functions/functions.util | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/src/functions/functions.util
> b/src/functions/functions.util
> index dd223f9..efcdfb3 100644
> --- a/src/functions/functions.util
> +++ b/src/functions/functions.util
> @@ -761,3 +761,29 @@ hex2dec() {
>  dec2hex() {
>  	printf "%02x\n" "${1}"
>  }
> +
> +copy() {
> +	# This function just copy config files
> +	assert [ $# -eq 2 ]
> +	local src=${1}
> +	local dst=${2}

Empty line after assert()

> +
> +	# Wo do the decleration and the initialisation in two lines
> to get the return code of fread
> +	local data
> +	data=$(fread "${src}")
> +	if [ ! $? -eq 0 ]; then
> +		log ERROR "Could not read data from ${src}"
> +		return ${EXIT_ERROR}
> +	fi

I am not a fan of this, but I am sure there is not a more elegant
solution.

> +
> +	# If the file exist we will overwrite it
> +	# fwrite would just append the contentet to the end of the
> file
> +	if [ -f ${dst} ]; then
> +		rm -f ${dst}
> +	fi
> +
> +	if ! fwrite "${dst}" "${data}"; then
> +		log ERROR "Could not write data to ${dst}"
> +		return ${EXIT_ERROR}
> +	fi
> +}

Here is where I don't really follow.

We never want to append something to an already existing file (maybe
fwrite shouldn't have been implemented as it was).

It's probably not good style to call "rm -f" from this script. I would
prefer unlink.

But before getting to that: Isn't it easiest just to implement it like
this:

  fread ${src} > ${dst}

This will just read the content and stream it into ${dst}. If you check
if $dst is a directory and similar stuff before you should be fine.

Way easier and should be able to copy even large files.

-Michael

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

LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KVmVyc2lvbjogR251UEcgdjIKCmlRSWNCQUFC
Q2dBR0JRSlpiUis1QUFvSkVJQjU4UDl2a0FrSEFPMFAvM1RzVklWWXF2YUUvZkp4OWRJVWZzVVEK
UG1qZE9XajF1TE9xb0ZHL3BSK3kzYkhPUmplU1RqdFN0S1hGRXJqbkMzQXBtZ1RCc1M2ZENiOUk4
YmExM2daYwpoK2ZBYXNJc0lVOUtTRnAzTGhQcUQwZnhqWEpyeC9zdUxzNTNJWFdXa1F2bTVRNmMv
QnlScmlaRnJqS2tBU1pxCkZvLzFIbHVkdkJBenBxa2hIM1Vuck9aSm5MOFlNenQ4NXYvTlQydjdX
ZjE1akd5SGp1L3orU1RnQW95L2hDQzcKemkyYmpqMG03SDE4ZFUwMlFaTnZrSEI1VUJFNWlEZHZD
R3FQRTF6dkptSUh6eXErQ1RLaG5ESThHREtVRXlyZApqNUtETjBFUnFBSGJXeEtFUkpaeGcySFdj
cWh1QTFEVFJlVUtmY0ozMU1DU1NCMzZGbHBPMmtyWEhJc1JnY0gwCkNaYlVPTytaMURWMS9QT3JO
Ulk5YlQvL1dkTFFHdWhPamxIQjEyODZkZWE5WFJPWkFFbzRUbHhiRysrUDRlNU0KVlIvNHZIaWtx
ZFRNcmdFcUltcE5PdmVrSTFUSXAxWDdNVElVWE8xM3lPOFJpR0VTZXFhcER1Vnk4TmJPM3VidApD
MnRnajFPVmZqL2kzS1c3QzRONDc5TWQ0VGxDazNkd1pJam5kMVNweVAxSC9NVmt4Yllob3J1WXBr
ZE15OVVHCmg4UWtWQkQ4MXM1WWYvU3VQNjJDc2tIM0l2ZThNK1ZUMjh3UFpRMDF5eGlOcW8wYUc0
a3I3bURFQUQ1aFZSK0oKbmttL0Q0aEI5ME9UcCtPWGk3dlMwTVI3ajFVd2hPSDg4ZHRYV05RMlJI
TEpMM3hNaDJRUytSN09HU29DYkpjbQpHejd0RE1zNUt1Mm5pNWxwRXlVQgo9MVZMTgotLS0tLUVO
RCBQR1AgU0lHTkFUVVJFLS0tLS0K

--===============2803310128360250959==--