[PATCH V2 1/3] base-files: sysupgrade: add tar.sh with helpers for building archives

Jo-Philipp Wich jo at mein.io
Wed Feb 28 00:51:14 PST 2024


Hi Rafał,

comments inline. Sorry for the bikeshedding ahead.

~ Jo

> [...]
> +
> +__tar_print_padding() {
> +	dd if=/dev/zero bs=$1 count=1 2>/dev/null

$1 may be 0 which is an invalid value for `bs=`:

   root at OpenWrt:~# dd bs=0
   dd: number 0 is not in 1..2147483647 range

A value of "0" is valid for `count=` though:

   root at OpenWrt:~# dd bs=1 count=0
   0+0 records in
   0+0 records out

So either revert to the previous version or hardcode `bs` to 1, pass the
desired size via `count=` and live with slightly less efficiency (which likely
does not matter at all).

> +}
> +
> +__tar_make_member() {
> +	local name="$1"
> +	local content="$2"

If you make this `local mtime=${3:-$(date +%s)}` you can rename this function 
to `tar_make_member_inline()` and drop the extra wrapper.

> +	local mtime="$3"
> +	local mode=644
> [...]
> +}
> +
> +tar_make_member_from_file() {

This `tar_make_member_from_file()` wrapper is unused and very trivial, the
only thing it adds is defaulting the mtime value to the mtime of the given
file path. I suggest to drop `tar_make_member_from_file()` entirely and to
rename  `tar_make_member_inline()` below to simply `tar_make_member()`.

Maybe also consider renaming it to `tar_print_member()` or
`tar_output_member()` since that is what it does.

> +	local name="$1"
> +
> +	__tar_make_member "$name" "$(cat $name)" "$(date +%s -r "$1")"
> +}
> +
> +tar_make_member_inline() {
> +	local name="$1"
> +	local content="$2"
> +	local mtime="${3:-$(date +%s)}"
> +
> +	__tar_make_member "$name" "$content" "$mtime"
> +}
> +

Analogous to the suggested name change above, the `tar_close()` function 
should probably be renamed to `tar_print_trailer()` or `tar_output_trailer()`
as well.

> +tar_close() {
> +	__tar_print_padding 1024
> +}



More information about the openwrt-devel mailing list