[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