mbox

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

Message ID 1500318318-18852-2-git-send-email-jonatan.schlag@ipfire.org
State Superseded
Headers

Message

Jonatan Schlag July 18, 2017, 5:05 a.m. UTC
  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@ipfire.org>
---
 src/functions/functions.util | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
  

Comments

Michael Tremer July 18, 2017, 6:36 a.m. UTC | #1
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@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