| Message ID | 20180128225838.1275-1-mforney@mforney.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-01-28, Michael Forney <mforney@mforney.org> wrote: > From: Michael Forney <forney@google.com> > > stat(1) is not standardized and different implementations have their own > (conflicting) flags for querying the size of a file. > > ls(1) provides the same information (value of st.st_size) in the 5th > column, > except when the file is a character or block device. Forgot to add Signed-off-by: Michael Forney <forney@google.com> > --- > scripts/Makefile.lib | 3 ++- > scripts/link-vmlinux.sh | 6 ++++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 7dee1da83e2a..24566d1c10d2 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -319,7 +319,8 @@ dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) > size_append = printf $(shell \ > dec_size=0; \ > for F in $1; do \ > - fsize=$$(stat -c "%s" $$F); \ > + set -- $$(ls -dn $$F); \ > + fsize=$$5; \ > dec_size=$$(expr $$dec_size + $$fsize); \ > done; \ > printf "%08x\n" $$dec_size | \ > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > index c0d129d7f430..4a5737647a79 100755 > --- a/scripts/link-vmlinux.sh > +++ b/scripts/link-vmlinux.sh > @@ -296,8 +296,10 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then > kallsyms .tmp_vmlinux2 .tmp_kallsyms2.o > > # step 3 > - size1=$(stat -c "%s" .tmp_kallsyms1.o) > - size2=$(stat -c "%s" .tmp_kallsyms2.o) > + set -- $(ls -dn .tmp_kallsyms1.o) > + size1=$5 > + set -- $(ls -dn .tmp_kallsyms2.o) > + size2=$5 > > if [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then > kallsymso=.tmp_kallsyms3.o > -- > 2.15.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-30 17:01 GMT+09:00 Michael Forney <mforney@mforney.org>: > On 2018-01-28, Michael Forney <mforney@mforney.org> wrote: >> From: Michael Forney <forney@google.com> >> >> stat(1) is not standardized and different implementations have their own >> (conflicting) flags for querying the size of a file. >> >> ls(1) provides the same information (value of st.st_size) in the 5th >> column, >> except when the file is a character or block device. > > Forgot to add > > Signed-off-by: Michael Forney <forney@google.com> > >> --- >> scripts/Makefile.lib | 3 ++- >> scripts/link-vmlinux.sh | 6 ++++-- >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib >> index 7dee1da83e2a..24566d1c10d2 100644 >> --- a/scripts/Makefile.lib >> +++ b/scripts/Makefile.lib >> @@ -319,7 +319,8 @@ dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) >> size_append = printf $(shell \ >> dec_size=0; \ >> for F in $1; do \ >> - fsize=$$(stat -c "%s" $$F); \ >> + set -- $$(ls -dn $$F); \ >> + fsize=$$5; \ >> dec_size=$$(expr $$dec_size + $$fsize); \ >> done; \ >> printf "%08x\n" $$dec_size | \ I sometimes see patches that address portability. But, the resulted code is generally ugly. (I would not say this is too ugly in this case.) Also, I see some more instances. If 'stat' is a problem, do you have an idea for alternative of "stat -c %Y"? $ git grep 'stat -c' Documentation/acpi/ssdt-overlays.txt:dd if=$tmp of="$EFIVARFS/$name-$guid" bs=$(stat -c %s $tmp) arch/arm/boot/deflate_xip_data.sh:file_end=$(stat -c "%s" "$XIPIMAGE") arch/blackfin/boot/install.sh: local stamp=$(stat -c %Y ${file} 2>/dev/null) arch/powerpc/boot/wrapper:strip_size=$(stat -c %s $vmz.$$) tools/testing/selftests/efivarfs/efivarfs.sh: if [ $(stat -c %s $file) -ne 5 ]; then >> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh >> index c0d129d7f430..4a5737647a79 100755 >> --- a/scripts/link-vmlinux.sh >> +++ b/scripts/link-vmlinux.sh >> @@ -296,8 +296,10 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then >> kallsyms .tmp_vmlinux2 .tmp_kallsyms2.o >> >> # step 3 >> - size1=$(stat -c "%s" .tmp_kallsyms1.o) >> - size2=$(stat -c "%s" .tmp_kallsyms2.o) >> + set -- $(ls -dn .tmp_kallsyms1.o) >> + size1=$5 >> + set -- $(ls -dn .tmp_kallsyms2.o) >> + size2=$5 >> >> if [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then >> kallsymso=.tmp_kallsyms3.o >> -- >> 2.15.1 >> >>
On 2018-02-06, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > I sometimes see patches that address portability. > But, the resulted code is generally ugly. > (I would not say this is too ugly in this case.) > > > Also, I see some more instances. > If 'stat' is a problem, > do you have an idea for alternative of "stat -c %Y"? I'm not aware of a portable alternative for that. However, it is only used in a single place for backing up uImage and System.map. If the `stat -c %Y` command produces no stdout, it uses the .old extension instead which seems like reasonable fallback behavior. > $ git grep 'stat -c' > Documentation/acpi/ssdt-overlays.txt:dd if=$tmp > of="$EFIVARFS/$name-$guid" bs=$(stat -c %s $tmp) > arch/arm/boot/deflate_xip_data.sh:file_end=$(stat -c "%s" "$XIPIMAGE") > arch/blackfin/boot/install.sh: local stamp=$(stat -c %Y ${file} > 2>/dev/null) > arch/powerpc/boot/wrapper:strip_size=$(stat -c %s $vmz.$$) > tools/testing/selftests/efivarfs/efivarfs.sh: if [ $(stat -c %s > $file) -ne 5 ]; then Would you like me to include those other instances of `stat -c %s` in this patch? I left them out for now because I wasn't sure if they needed to be sent to various other maintainers as separate patches. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-02-06 18:13 GMT+09:00 Michael Forney <mforney@mforney.org>: > On 2018-02-06, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> I sometimes see patches that address portability. >> But, the resulted code is generally ugly. >> (I would not say this is too ugly in this case.) >> >> >> Also, I see some more instances. >> If 'stat' is a problem, >> do you have an idea for alternative of "stat -c %Y"? > > I'm not aware of a portable alternative for that. However, it is only > used in a single place for backing up uImage and System.map. If the > `stat -c %Y` command produces no stdout, it uses the .old extension > instead which seems like reasonable fallback behavior. Ah, right. >> $ git grep 'stat -c' >> Documentation/acpi/ssdt-overlays.txt:dd if=$tmp >> of="$EFIVARFS/$name-$guid" bs=$(stat -c %s $tmp) >> arch/arm/boot/deflate_xip_data.sh:file_end=$(stat -c "%s" "$XIPIMAGE") >> arch/blackfin/boot/install.sh: local stamp=$(stat -c %Y ${file} >> 2>/dev/null) >> arch/powerpc/boot/wrapper:strip_size=$(stat -c %s $vmz.$$) >> tools/testing/selftests/efivarfs/efivarfs.sh: if [ $(stat -c %s >> $file) -ne 5 ]; then > > Would you like me to include those other instances of `stat -c %s` in > this patch? I left them out for now because I wasn't sure if they > needed to be sent to various other maintainers as separate patches. I want all instances written in the same way once we make a decision. BTW, is the output format of ls(1) standardized? I am not familiar with this area. Also, I am not a big fan of the mysterious two lines code. (We will have some more if we fix the rest of the instances) Perhaps, put set -- $(ls -dn $1) echo $5 into "scripts/file-size.sh" (any other suitable file name is OK) then, call it like this? size1=$($CONFIG_SHELL $srctree/scripts/file-size.sh .tmp_kallsyms1.o) size2=$($CONFIG_SHELL $srctree/scripts/file-size.sh .tmp_kallsyms2.o) Is this less uglier?
On 2018-02-06, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-06 18:13 GMT+09:00 Michael Forney <mforney@mforney.org>: >> Would you like me to include those other instances of `stat -c %s` in >> this patch? I left them out for now because I wasn't sure if they >> needed to be sent to various other maintainers as separate patches. > > I want all instances written in the same way > once we make a decision. > > BTW, is the output format of ls(1) standardized? > I am not familiar with this area. Yes, it is[0]. The -d option turns on -l, which writes lines formatted like "%s %u %s %s %u %s %s\n", <file mode>, <number of links>, <owner name>, <group name>, <size>, <date and time>, <pathname> but instead of writing the <owner name> and <group name>, it writes the numeric owner and group IDs (this avoids /etc/passwd and /etc/group lookups as well as potential field splitting issues). The <size> field is specified as "the value that would be returned for the file in the st_size field of struct stat". > Also, I am not a big fan of the mysterious two lines code. > (We will have some more if we fix the rest of the instances) > > > Perhaps, put > set -- $(ls -dn $1) > echo $5 > > into "scripts/file-size.sh" (any other suitable file name is OK) > then, call it like this? > > size1=$($CONFIG_SHELL $srctree/scripts/file-size.sh .tmp_kallsyms1.o) > size2=$($CONFIG_SHELL $srctree/scripts/file-size.sh .tmp_kallsyms2.o) > > > Is this less uglier? This sounds reasonable to me. I will make this change. [0] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html#tag_20_73_10 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-02-06 19:29 GMT+09:00 Michael Forney <mforney@mforney.org>: > On 2018-02-06, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> 2018-02-06 18:13 GMT+09:00 Michael Forney <mforney@mforney.org>: >>> Would you like me to include those other instances of `stat -c %s` in >>> this patch? I left them out for now because I wasn't sure if they >>> needed to be sent to various other maintainers as separate patches. >> >> I want all instances written in the same way >> once we make a decision. >> >> BTW, is the output format of ls(1) standardized? >> I am not familiar with this area. > > Yes, it is[0]. The -d option turns on -l, which writes lines formatted like > > "%s %u %s %s %u %s %s\n", <file mode>, <number of links>, > <owner name>, <group name>, <size>, <date and time>, > <pathname> OK. Please add this evidence with the link into the commit log. > but instead of writing the <owner name> and <group name>, it writes > the numeric owner and group IDs (this avoids /etc/passwd and > /etc/group lookups as well as potential field splitting issues). > > The <size> field is specified as "the value that would be returned for > the file in the st_size field of struct stat". > >> Also, I am not a big fan of the mysterious two lines code. >> (We will have some more if we fix the rest of the instances) >> >> >> Perhaps, put >> set -- $(ls -dn $1) >> echo $5 >> >> into "scripts/file-size.sh" (any other suitable file name is OK) >> then, call it like this? >> >> size1=$($CONFIG_SHELL $srctree/scripts/file-size.sh .tmp_kallsyms1.o) >> size2=$($CONFIG_SHELL $srctree/scripts/file-size.sh .tmp_kallsyms2.o) >> >> >> Is this less uglier? > > This sounds reasonable to me. I will make this change. > > [0] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html#tag_20_73_10 > -- > To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 7dee1da83e2a..24566d1c10d2 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -319,7 +319,8 @@ dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) size_append = printf $(shell \ dec_size=0; \ for F in $1; do \ - fsize=$$(stat -c "%s" $$F); \ + set -- $$(ls -dn $$F); \ + fsize=$$5; \ dec_size=$$(expr $$dec_size + $$fsize); \ done; \ printf "%08x\n" $$dec_size | \ diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh index c0d129d7f430..4a5737647a79 100755 --- a/scripts/link-vmlinux.sh +++ b/scripts/link-vmlinux.sh @@ -296,8 +296,10 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then kallsyms .tmp_vmlinux2 .tmp_kallsyms2.o # step 3 - size1=$(stat -c "%s" .tmp_kallsyms1.o) - size2=$(stat -c "%s" .tmp_kallsyms2.o) + set -- $(ls -dn .tmp_kallsyms1.o) + size1=$5 + set -- $(ls -dn .tmp_kallsyms2.o) + size2=$5 if [ $size1 -ne $size2 ] || [ -n "${KALLSYMS_EXTRA_PASS}" ]; then kallsymso=.tmp_kallsyms3.o
From: Michael Forney <forney@google.com> stat(1) is not standardized and different implementations have their own (conflicting) flags for querying the size of a file. ls(1) provides the same information (value of st.st_size) in the 5th column, except when the file is a character or block device. --- scripts/Makefile.lib | 3 ++- scripts/link-vmlinux.sh | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-)