Re: [PATCH v2 1/4] util: add copy function

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

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

Hi,

there is a few bugs in this function...

On 15/07/2017 03:19 PM, Jonatan Schlag wrote:
> Adds a nice function to copy simple configuration files.
> 
> 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 0a9b3d6..a6c98c6 100644
> --- a/src/functions/functions.util
> +++ b/src/functions/functions.util
> @@ -766,3 +766,29 @@ hex2dec() {
>  dec2hex() {
>  	printf "%02x\n" "${1}"
>  }
> +
> +
> +copy() {
> +	# This function just copy to config files
> +	assert [ $# -eq 2 ]
> +	src=${1}
> +	dst=${2}

The local keyword is missing here.

> +	local data=$(fread ${src})
> +	if [ ! $? -eq 0 ]; then
> +		log ERROR "Could not read data from ${src}"
> +		return ${EXIT_ERROR}
> +	fi

Are you sure that $? is set to the exit code of fread here?

It might be that the shell sets that if the variable assignment
to local was successful (which it always is). I think you might
need to use the ${PIPESTATUS} array here.

> +	if [  -e ${dst} ]; then
> +		log ERROR "Destination ${dst} already exist"
> +		return ${EXIT_ERROR}
> +	else
> +		touch ${dst}
> +	fi

Is that a problem when the destination already exists? I think
we should allow copy() to overwrite files.

What we do need to check though is if $dst is a directory. In that
case we cannot write to it.

And there is no need to touch the file when it does not exist,
yet. fwrite will create it (I hope - if not change fwrite).

> +	if ! fwrite ${dst} "${data}"; then
> +		log ERROR "Could not write data to ${dst}"
> +		return ${EXIT_ERROR}
> +	fi
> +}

-Michael