Re: [PATCH v3 2/5] util: add new function copy

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 2566 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]