[kvm-unit-tests] Makefiles: Remove the executable bit from the .elf and .flat files
diff mbox series

Message ID 1532593312-19640-1-git-send-email-thuth@redhat.com
State New
Headers show
Series
  • [kvm-unit-tests] Makefiles: Remove the executable bit from the .elf and .flat files
Related show

Commit Message

Thomas Huth July 26, 2018, 8:21 a.m. UTC
The .elf and .flat files are not runnable on the host operating system,
so they should not be marked as executable. As we discovered recently,
the executable flag can also cause trouble when the files are packaged
in an .rpm file, since rpm "colors" the package differently if there
are 32-bit or 64-bit executables in the package (for multilib support).

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arm/Makefile.common     | 2 ++
 powerpc/Makefile.common | 2 ++
 s390x/Makefile          | 1 +
 x86/Makefile.common     | 2 ++
 4 files changed, 7 insertions(+)

Comments

Paolo Bonzini July 26, 2018, 8:24 a.m. UTC | #1
On 26/07/2018 10:21, Thomas Huth wrote:
> The .elf and .flat files are not runnable on the host operating system,
> so they should not be marked as executable. As we discovered recently,
> the executable flag can also cause trouble when the files are packaged
> in an .rpm file, since rpm "colors" the package differently if there
> are 32-bit or 64-bit executables in the package (for multilib support).

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
David Hildenbrand July 26, 2018, 8:24 a.m. UTC | #2
On 26.07.2018 10:21, Thomas Huth wrote:
> The .elf and .flat files are not runnable on the host operating system,
> so they should not be marked as executable. As we discovered recently,
> the executable flag can also cause trouble when the files are packaged
> in an .rpm file, since rpm "colors" the package differently if there
> are 32-bit or 64-bit executables in the package (for multilib support).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  arm/Makefile.common     | 2 ++
>  powerpc/Makefile.common | 2 ++
>  s390x/Makefile          | 1 +
>  x86/Makefile.common     | 2 ++
>  4 files changed, 7 insertions(+)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 1cf9cdc..e62a978 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
>  		$(filter %.o, $^) $(FLATLIBS) \
>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O binary $^ $@
> +	chmod a-x $@
>  
>  $(libeabi): $(eabiobjs)
>  	$(AR) rcs $@ $^
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 81c5074..af6b9d8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  	@echo -n Checking $@ for unsupported reloc types...
>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
>  		false;							\
> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> +	chmod a-x $@
>  
>  powerpc_clean: libfdt_clean asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 6546a02..6b32f2c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  
>  arch_clean: asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 7dcf8c2..0d64284 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
>  		$(filter %.o, $^) $(FLATLIBS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O elf32-i386 $^ $@
> +	chmod a-x $@
>  
>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
> 

Reviewed-by: David Hildenbrand <david@redhat.com>
Laszlo Ersek July 26, 2018, 9:10 a.m. UTC | #3
On 07/26/18 10:21, Thomas Huth wrote:
> The .elf and .flat files are not runnable on the host operating system,
> so they should not be marked as executable. As we discovered recently,
> the executable flag can also cause trouble when the files are packaged
> in an .rpm file, since rpm "colors" the package differently if there
> are 32-bit or 64-bit executables in the package (for multilib support).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  arm/Makefile.common     | 2 ++
>  powerpc/Makefile.common | 2 ++
>  s390x/Makefile          | 1 +
>  x86/Makefile.common     | 2 ++
>  4 files changed, 7 insertions(+)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 1cf9cdc..e62a978 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
>  		$(filter %.o, $^) $(FLATLIBS) \
>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O binary $^ $@
> +	chmod a-x $@
>  
>  $(libeabi): $(eabiobjs)
>  	$(AR) rcs $@ $^
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 81c5074..af6b9d8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  	@echo -n Checking $@ for unsupported reloc types...
>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
>  		false;							\
> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> +	chmod a-x $@
>  
>  powerpc_clean: libfdt_clean asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 6546a02..6b32f2c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  
>  arch_clean: asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 7dcf8c2..0d64284 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
>  		$(filter %.o, $^) $(FLATLIBS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O elf32-i386 $^ $@
> +	chmod a-x $@
>  
>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thank you Thomas!
Laszlo
Andrew Jones July 26, 2018, 10:11 a.m. UTC | #4
On Thu, Jul 26, 2018 at 10:21:52AM +0200, Thomas Huth wrote:
> The .elf and .flat files are not runnable on the host operating system,
> so they should not be marked as executable. As we discovered recently,
> the executable flag can also cause trouble when the files are packaged
> in an .rpm file, since rpm "colors" the package differently if there
> are 32-bit or 64-bit executables in the package (for multilib support).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  arm/Makefile.common     | 2 ++
>  powerpc/Makefile.common | 2 ++
>  s390x/Makefile          | 1 +
>  x86/Makefile.common     | 2 ++
>  4 files changed, 7 insertions(+)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 1cf9cdc..e62a978 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
>  		$(filter %.o, $^) $(FLATLIBS) \
>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O binary $^ $@
> +	chmod a-x $@
>  
>  $(libeabi): $(eabiobjs)
>  	$(AR) rcs $@ $^
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 81c5074..af6b9d8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  	@echo -n Checking $@ for unsupported reloc types...
>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
>  		false;							\
> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> +	chmod a-x $@
>  
>  powerpc_clean: libfdt_clean asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 6546a02..6b32f2c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  
>  arch_clean: asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 7dcf8c2..0d64284 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
>  		$(filter %.o, $^) $(FLATLIBS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O elf32-i386 $^ $@
> +	chmod a-x $@
>  
>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
> -- 
> 1.8.3.1
>

Can you please add '@' to the front of all these chmod lines? Otherwise 
each unit test compilation gets two more lines of uninteresting output
when compiling.

Thanks,
drew
Thomas Huth July 26, 2018, 1:58 p.m. UTC | #5
On 07/26/2018 12:11 PM, Andrew Jones wrote:
> On Thu, Jul 26, 2018 at 10:21:52AM +0200, Thomas Huth wrote:
>> The .elf and .flat files are not runnable on the host operating system,
>> so they should not be marked as executable. As we discovered recently,
>> the executable flag can also cause trouble when the files are packaged
>> in an .rpm file, since rpm "colors" the package differently if there
>> are 32-bit or 64-bit executables in the package (for multilib support).
>>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  arm/Makefile.common     | 2 ++
>>  powerpc/Makefile.common | 2 ++
>>  s390x/Makefile          | 1 +
>>  x86/Makefile.common     | 2 ++
>>  4 files changed, 7 insertions(+)
>>
>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>> index 1cf9cdc..e62a978 100644
>> --- a/arm/Makefile.common
>> +++ b/arm/Makefile.common
>> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
>>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
>>  		$(filter %.o, $^) $(FLATLIBS) \
>>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
>> +	chmod a-x $@
>>  
>>  %.flat: %.elf
>>  	$(OBJCOPY) -O binary $^ $@
>> +	chmod a-x $@
>>  
>>  $(libeabi): $(eabiobjs)
>>  	$(AR) rcs $@ $^
>> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
>> index 81c5074..af6b9d8 100644
>> --- a/powerpc/Makefile.common
>> +++ b/powerpc/Makefile.common
>> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
>>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
>>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>>  	$(RM) $(@:.elf=.aux.o)
>> +	chmod a-x $@
>>  	@echo -n Checking $@ for unsupported reloc types...
>>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
>>  		false;							\
>> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
>>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
>>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
>>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
>> +	chmod a-x $@
>>  
>>  powerpc_clean: libfdt_clean asm_offsets_clean
>>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 6546a02..6b32f2c 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
>>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
>>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>>  	$(RM) $(@:.elf=.aux.o)
>> +	chmod a-x $@
>>  
>>  arch_clean: asm_offsets_clean
>>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
>> diff --git a/x86/Makefile.common b/x86/Makefile.common
>> index 7dcf8c2..0d64284 100644
>> --- a/x86/Makefile.common
>> +++ b/x86/Makefile.common
>> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
>>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
>>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
>>  		$(filter %.o, $^) $(FLATLIBS)
>> +	chmod a-x $@
>>  
>>  %.flat: %.elf
>>  	$(OBJCOPY) -O elf32-i386 $^ $@
>> +	chmod a-x $@
>>  
>>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
>> -- 
>> 1.8.3.1
>>
> 
> Can you please add '@' to the front of all these chmod lines? Otherwise 
> each unit test compilation gets two more lines of uninteresting output
> when compiling.

I don't think that we should arbitrarily add a '@' to lines which might
be uninteresting at a first glance. What might be uninteresting for you,
might be helpful for others to see what is going on. If you're not
interested in the full output of make, simply compile with the "-s" option.

 Thomas
Andrew Jones July 26, 2018, 2:22 p.m. UTC | #6
On Thu, Jul 26, 2018 at 03:58:45PM +0200, Thomas Huth wrote:
> On 07/26/2018 12:11 PM, Andrew Jones wrote:
> > On Thu, Jul 26, 2018 at 10:21:52AM +0200, Thomas Huth wrote:
> >> The .elf and .flat files are not runnable on the host operating system,
> >> so they should not be marked as executable. As we discovered recently,
> >> the executable flag can also cause trouble when the files are packaged
> >> in an .rpm file, since rpm "colors" the package differently if there
> >> are 32-bit or 64-bit executables in the package (for multilib support).
> >>
> >> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  arm/Makefile.common     | 2 ++
> >>  powerpc/Makefile.common | 2 ++
> >>  s390x/Makefile          | 1 +
> >>  x86/Makefile.common     | 2 ++
> >>  4 files changed, 7 insertions(+)
> >>
> >> diff --git a/arm/Makefile.common b/arm/Makefile.common
> >> index 1cf9cdc..e62a978 100644
> >> --- a/arm/Makefile.common
> >> +++ b/arm/Makefile.common
> >> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
> >>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
> >>  		$(filter %.o, $^) $(FLATLIBS) \
> >>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
> >> +	chmod a-x $@
> >>  
> >>  %.flat: %.elf
> >>  	$(OBJCOPY) -O binary $^ $@
> >> +	chmod a-x $@
> >>  
> >>  $(libeabi): $(eabiobjs)
> >>  	$(AR) rcs $@ $^
> >> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> >> index 81c5074..af6b9d8 100644
> >> --- a/powerpc/Makefile.common
> >> +++ b/powerpc/Makefile.common
> >> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
> >>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
> >>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
> >>  	$(RM) $(@:.elf=.aux.o)
> >> +	chmod a-x $@
> >>  	@echo -n Checking $@ for unsupported reloc types...
> >>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
> >>  		false;							\
> >> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
> >>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
> >>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
> >>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> >> +	chmod a-x $@
> >>  
> >>  powerpc_clean: libfdt_clean asm_offsets_clean
> >>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> >> diff --git a/s390x/Makefile b/s390x/Makefile
> >> index 6546a02..6b32f2c 100644
> >> --- a/s390x/Makefile
> >> +++ b/s390x/Makefile
> >> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
> >>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
> >>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
> >>  	$(RM) $(@:.elf=.aux.o)
> >> +	chmod a-x $@
> >>  
> >>  arch_clean: asm_offsets_clean
> >>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
> >> diff --git a/x86/Makefile.common b/x86/Makefile.common
> >> index 7dcf8c2..0d64284 100644
> >> --- a/x86/Makefile.common
> >> +++ b/x86/Makefile.common
> >> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
> >>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
> >>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
> >>  		$(filter %.o, $^) $(FLATLIBS)
> >> +	chmod a-x $@
> >>  
> >>  %.flat: %.elf
> >>  	$(OBJCOPY) -O elf32-i386 $^ $@
> >> +	chmod a-x $@
> >>  
> >>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
> >>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
> >> -- 
> >> 1.8.3.1
> >>
> > 
> > Can you please add '@' to the front of all these chmod lines? Otherwise 
> > each unit test compilation gets two more lines of uninteresting output
> > when compiling.
> 
> I don't think that we should arbitrarily add a '@' to lines which might
> be uninteresting at a first glance. What might be uninteresting for you,
> might be helpful for others to see what is going on. If you're not
> interested in the full output of make, simply compile with the "-s" option.
>

It's not arbitrary. Currently when I build I see lines I want to see. I
don't want to use -s to turn them off. With this patch I'll see the lines
I want and another 2 lines per unit test that I don't. How can hiding
these lines be harmful to a developer? They don't affect the build or
run in any way, and, as the commit message says, it's only being done
for packaging in the first place.

Thanks,
drew
Laszlo Ersek July 26, 2018, 2:59 p.m. UTC | #7
On 07/26/18 16:22, Andrew Jones wrote:
> On Thu, Jul 26, 2018 at 03:58:45PM +0200, Thomas Huth wrote:
>> On 07/26/2018 12:11 PM, Andrew Jones wrote:
>>> On Thu, Jul 26, 2018 at 10:21:52AM +0200, Thomas Huth wrote:
>>>> The .elf and .flat files are not runnable on the host operating system,
>>>> so they should not be marked as executable. As we discovered recently,
>>>> the executable flag can also cause trouble when the files are packaged
>>>> in an .rpm file, since rpm "colors" the package differently if there
>>>> are 32-bit or 64-bit executables in the package (for multilib support).
>>>>
>>>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  arm/Makefile.common     | 2 ++
>>>>  powerpc/Makefile.common | 2 ++
>>>>  s390x/Makefile          | 1 +
>>>>  x86/Makefile.common     | 2 ++
>>>>  4 files changed, 7 insertions(+)
>>>>
>>>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>>>> index 1cf9cdc..e62a978 100644
>>>> --- a/arm/Makefile.common
>>>> +++ b/arm/Makefile.common
>>>> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
>>>>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
>>>>  		$(filter %.o, $^) $(FLATLIBS) \
>>>>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
>>>> +	chmod a-x $@
>>>>  
>>>>  %.flat: %.elf
>>>>  	$(OBJCOPY) -O binary $^ $@
>>>> +	chmod a-x $@
>>>>  
>>>>  $(libeabi): $(eabiobjs)
>>>>  	$(AR) rcs $@ $^
>>>> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
>>>> index 81c5074..af6b9d8 100644
>>>> --- a/powerpc/Makefile.common
>>>> +++ b/powerpc/Makefile.common
>>>> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
>>>>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
>>>>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>>>>  	$(RM) $(@:.elf=.aux.o)
>>>> +	chmod a-x $@
>>>>  	@echo -n Checking $@ for unsupported reloc types...
>>>>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
>>>>  		false;							\
>>>> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
>>>>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
>>>>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
>>>>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
>>>> +	chmod a-x $@
>>>>  
>>>>  powerpc_clean: libfdt_clean asm_offsets_clean
>>>>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
>>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>>> index 6546a02..6b32f2c 100644
>>>> --- a/s390x/Makefile
>>>> +++ b/s390x/Makefile
>>>> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
>>>>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
>>>>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>>>>  	$(RM) $(@:.elf=.aux.o)
>>>> +	chmod a-x $@
>>>>  
>>>>  arch_clean: asm_offsets_clean
>>>>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
>>>> diff --git a/x86/Makefile.common b/x86/Makefile.common
>>>> index 7dcf8c2..0d64284 100644
>>>> --- a/x86/Makefile.common
>>>> +++ b/x86/Makefile.common
>>>> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
>>>>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
>>>>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
>>>>  		$(filter %.o, $^) $(FLATLIBS)
>>>> +	chmod a-x $@
>>>>  
>>>>  %.flat: %.elf
>>>>  	$(OBJCOPY) -O elf32-i386 $^ $@
>>>> +	chmod a-x $@
>>>>  
>>>>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>>>>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
>>>> -- 
>>>> 1.8.3.1
>>>>
>>>
>>> Can you please add '@' to the front of all these chmod lines? Otherwise 
>>> each unit test compilation gets two more lines of uninteresting output
>>> when compiling.
>>
>> I don't think that we should arbitrarily add a '@' to lines which might
>> be uninteresting at a first glance. What might be uninteresting for you,
>> might be helpful for others to see what is going on. If you're not
>> interested in the full output of make, simply compile with the "-s" option.
>>
> 
> It's not arbitrary. Currently when I build I see lines I want to see. I
> don't want to use -s to turn them off. With this patch I'll see the lines
> I want and another 2 lines per unit test that I don't. How can hiding
> these lines be harmful to a developer? They don't affect the build or
> run in any way, and, as the commit message says, it's only being done
> for packaging in the first place.

(I'm entirely neutral on this question.) What distinguishes the lines
you prefer to see from the new chmod lines? What is the information in
the pre-existent lines that is useful to see, and that the new chmod
lines lack?

I've only ever built kvm-unit-tests once (and an old version at that);
the lines that were interesting to me were:

  gcc -mno-red-zone -m64 -O1 -g -MMD \
    -MF x86/.tscdeadline_latency.d -Wall  -fomit-frame-pointer \
    -fno-stack-protector -nostdlib -o x86/tscdeadline_latency.elf \
    -Wl,-T,x86/flat.lds \
    x86/tscdeadline_latency.o x86/cstart64.o lib/libcflat.a \
    /usr/lib/gcc/x86_64-redhat-linux/4.8.5/libgcc.a

  objcopy -O elf32-i386 \
    x86/tscdeadline_latency.elf \
    x86/tscdeadline_latency.flat

Here gcc and objcopy produce files with the executable mode bits set,
which is likely useful in the general case, but not useful in the
specific case. If we hide "chmod" with @, then someone used to objcopy
producing executable files might be surprised that the build product is
not executable after all -- the "make" output will not explain that.

Again, I'm perfectly fine both with and without silencing chmod, I'm
just curious about the information that sets the currently visible lines
apart from the chmod lines.

Thanks!
Laszlo
Andrew Jones July 26, 2018, 3:56 p.m. UTC | #8
On Thu, Jul 26, 2018 at 04:59:06PM +0200, Laszlo Ersek wrote:
> On 07/26/18 16:22, Andrew Jones wrote:
> > On Thu, Jul 26, 2018 at 03:58:45PM +0200, Thomas Huth wrote:
> >> On 07/26/2018 12:11 PM, Andrew Jones wrote:
> >>> On Thu, Jul 26, 2018 at 10:21:52AM +0200, Thomas Huth wrote:
> >>>> The .elf and .flat files are not runnable on the host operating system,
> >>>> so they should not be marked as executable. As we discovered recently,
> >>>> the executable flag can also cause trouble when the files are packaged
> >>>> in an .rpm file, since rpm "colors" the package differently if there
> >>>> are 32-bit or 64-bit executables in the package (for multilib support).
> >>>>
> >>>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>> ---
> >>>>  arm/Makefile.common     | 2 ++
> >>>>  powerpc/Makefile.common | 2 ++
> >>>>  s390x/Makefile          | 1 +
> >>>>  x86/Makefile.common     | 2 ++
> >>>>  4 files changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/arm/Makefile.common b/arm/Makefile.common
> >>>> index 1cf9cdc..e62a978 100644
> >>>> --- a/arm/Makefile.common
> >>>> +++ b/arm/Makefile.common
> >>>> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
> >>>>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
> >>>>  		$(filter %.o, $^) $(FLATLIBS) \
> >>>>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
> >>>> +	chmod a-x $@
> >>>>  
> >>>>  %.flat: %.elf
> >>>>  	$(OBJCOPY) -O binary $^ $@
> >>>> +	chmod a-x $@
> >>>>  
> >>>>  $(libeabi): $(eabiobjs)
> >>>>  	$(AR) rcs $@ $^
> >>>> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> >>>> index 81c5074..af6b9d8 100644
> >>>> --- a/powerpc/Makefile.common
> >>>> +++ b/powerpc/Makefile.common
> >>>> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
> >>>>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
> >>>>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
> >>>>  	$(RM) $(@:.elf=.aux.o)
> >>>> +	chmod a-x $@
> >>>>  	@echo -n Checking $@ for unsupported reloc types...
> >>>>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
> >>>>  		false;							\
> >>>> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
> >>>>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
> >>>>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
> >>>>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> >>>> +	chmod a-x $@
> >>>>  
> >>>>  powerpc_clean: libfdt_clean asm_offsets_clean
> >>>>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> >>>> diff --git a/s390x/Makefile b/s390x/Makefile
> >>>> index 6546a02..6b32f2c 100644
> >>>> --- a/s390x/Makefile
> >>>> +++ b/s390x/Makefile
> >>>> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
> >>>>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
> >>>>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
> >>>>  	$(RM) $(@:.elf=.aux.o)
> >>>> +	chmod a-x $@
> >>>>  
> >>>>  arch_clean: asm_offsets_clean
> >>>>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
> >>>> diff --git a/x86/Makefile.common b/x86/Makefile.common
> >>>> index 7dcf8c2..0d64284 100644
> >>>> --- a/x86/Makefile.common
> >>>> +++ b/x86/Makefile.common
> >>>> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
> >>>>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
> >>>>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
> >>>>  		$(filter %.o, $^) $(FLATLIBS)
> >>>> +	chmod a-x $@
> >>>>  
> >>>>  %.flat: %.elf
> >>>>  	$(OBJCOPY) -O elf32-i386 $^ $@
> >>>> +	chmod a-x $@
> >>>>  
> >>>>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
> >>>>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
> >>>> -- 
> >>>> 1.8.3.1
> >>>>
> >>>
> >>> Can you please add '@' to the front of all these chmod lines? Otherwise 
> >>> each unit test compilation gets two more lines of uninteresting output
> >>> when compiling.
> >>
> >> I don't think that we should arbitrarily add a '@' to lines which might
> >> be uninteresting at a first glance. What might be uninteresting for you,
> >> might be helpful for others to see what is going on. If you're not
> >> interested in the full output of make, simply compile with the "-s" option.
> >>
> > 
> > It's not arbitrary. Currently when I build I see lines I want to see. I
> > don't want to use -s to turn them off. With this patch I'll see the lines
> > I want and another 2 lines per unit test that I don't. How can hiding
> > these lines be harmful to a developer? They don't affect the build or
> > run in any way, and, as the commit message says, it's only being done
> > for packaging in the first place.
> 
> (I'm entirely neutral on this question.) What distinguishes the lines
> you prefer to see from the new chmod lines? What is the information in
> the pre-existent lines that is useful to see, and that the new chmod
> lines lack?
> 
> I've only ever built kvm-unit-tests once (and an old version at that);
> the lines that were interesting to me were:
> 
>   gcc -mno-red-zone -m64 -O1 -g -MMD \
>     -MF x86/.tscdeadline_latency.d -Wall  -fomit-frame-pointer \
>     -fno-stack-protector -nostdlib -o x86/tscdeadline_latency.elf \
>     -Wl,-T,x86/flat.lds \
>     x86/tscdeadline_latency.o x86/cstart64.o lib/libcflat.a \
>     /usr/lib/gcc/x86_64-redhat-linux/4.8.5/libgcc.a
> 
>   objcopy -O elf32-i386 \
>     x86/tscdeadline_latency.elf \
>     x86/tscdeadline_latency.flat

Those are the lines interesting to me as well. And for arm, before
this patch, those are pretty much the only lines that are currently
output.

> 
> Here gcc and objcopy produce files with the executable mode bits set,
> which is likely useful in the general case, but not useful in the
> specific case. If we hide "chmod" with @, then someone used to objcopy
> producing executable files might be surprised that the build product is
> not executable after all -- the "make" output will not explain that.

They'll also be surprised that they have a useless .flat file unless
they know what they're doing with it. If you're running tests with the
scripts then you'll never know if the files have exec bits set or not.
If you're not running with the scripts, then I guess you already know
what you're doing. Just my opinion of course...

> 
> Again, I'm perfectly fine both with and without silencing chmod, I'm
> just curious about the information that sets the currently visible lines
> apart from the chmod lines.
>

I'll never bother to copy+paste chmod lines from the output to use
for a manual compile (should I want to experiment with cflags or
something). To me, the chmod lines will always just be noise.

Thanks,
drew
Laszlo Ersek July 27, 2018, 1:39 p.m. UTC | #9
On 07/26/18 10:21, Thomas Huth wrote:
> The .elf and .flat files are not runnable on the host operating system,
> so they should not be marked as executable. As we discovered recently,
> the executable flag can also cause trouble when the files are packaged
> in an .rpm file, since rpm "colors" the package differently if there
> are 32-bit or 64-bit executables in the package (for multilib support).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  arm/Makefile.common     | 2 ++
>  powerpc/Makefile.common | 2 ++
>  s390x/Makefile          | 1 +
>  x86/Makefile.common     | 2 ++
>  4 files changed, 7 insertions(+)
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 1cf9cdc..e62a978 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -73,9 +73,11 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
>  		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
>  		$(filter %.o, $^) $(FLATLIBS) \
>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O binary $^ $@
> +	chmod a-x $@
>  
>  $(libeabi): $(eabiobjs)
>  	$(AR) rcs $@ $^
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 81c5074..af6b9d8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -54,6 +54,7 @@ FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  	@echo -n Checking $@ for unsupported reloc types...
>  	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
>  		false;							\
> @@ -70,6 +71,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
>  $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
>  $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
>  	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> +	chmod a-x $@
>  
>  powerpc_clean: libfdt_clean asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 6546a02..6b32f2c 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -53,6 +53,7 @@ FLATLIBS = $(libcflat)
>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
>  		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
>  	$(RM) $(@:.elf=.aux.o)
> +	chmod a-x $@
>  
>  arch_clean: asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
> diff --git a/x86/Makefile.common b/x86/Makefile.common
> index 7dcf8c2..0d64284 100644
> --- a/x86/Makefile.common
> +++ b/x86/Makefile.common
> @@ -42,9 +42,11 @@ FLATLIBS = lib/libcflat.a $(libgcc)
>  %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
>  	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
>  		$(filter %.o, $^) $(FLATLIBS)
> +	chmod a-x $@
>  
>  %.flat: %.elf
>  	$(OBJCOPY) -O elf32-i386 $^ $@
> +	chmod a-x $@
>  
>  tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>                 $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \
> 

For integrity's sake, I must add the following:

While I think this change would be technically correct (= the file
should not be advertised, after build, as executable), it now seems that
the patch would not help with the packaging issue.

It appears that the RPM build infrastructure, at least in Fedora28+,
ignores the x file mode bits when it decides to color a file as "1" (=
32-bit ELF).

https://bugzilla.redhat.com/show_bug.cgi?id=1608852#c2

This behavior looks *entirely inconsistent* with how RPM building seems
to work in RHEL7, but that's the situation we got, apparently.

I believe this finding makes the commit message *in part* dubious.
Therefore, personally I'd be fine if we dropped the patch (although I do
believe what it does is still correct, just not with the original
justification).

Thanks,
Laszlo
David Hildenbrand July 27, 2018, 1:46 p.m. UTC | #10
> For integrity's sake, I must add the following:
> 
> While I think this change would be technically correct (= the file
> should not be advertised, after build, as executable), it now seems that
> the patch would not help with the packaging issue.
> 
> It appears that the RPM build infrastructure, at least in Fedora28+,
> ignores the x file mode bits when it decides to color a file as "1" (=
> 32-bit ELF).
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1608852#c2
> 
> This behavior looks *entirely inconsistent* with how RPM building seems
> to work in RHEL7, but that's the situation we got, apparently.
> 
> I believe this finding makes the commit message *in part* dubious.
> Therefore, personally I'd be fine if we dropped the patch (although I do
> believe what it does is still correct, just not with the original
> justification).

From a !packaging point of view, this is the right thing to do. These
are binary blobs to be loaded into a guest VM, not executables.

Thomas, maybe simply drop that part about packaging from the commit message.

> 
> Thanks,
> Laszlo
>
Laszlo Ersek July 27, 2018, 3:25 p.m. UTC | #11
On 07/27/18 15:46, David Hildenbrand wrote:
>> For integrity's sake, I must add the following:
>>
>> While I think this change would be technically correct (= the file
>> should not be advertised, after build, as executable), it now seems that
>> the patch would not help with the packaging issue.
>>
>> It appears that the RPM build infrastructure, at least in Fedora28+,
>> ignores the x file mode bits when it decides to color a file as "1" (=
>> 32-bit ELF).
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1608852#c2
>>
>> This behavior looks *entirely inconsistent* with how RPM building seems
>> to work in RHEL7, but that's the situation we got, apparently.
>>
>> I believe this finding makes the commit message *in part* dubious.
>> Therefore, personally I'd be fine if we dropped the patch (although I do
>> believe what it does is still correct, just not with the original
>> justification).
> 
> From a !packaging point of view, this is the right thing to do. These
> are binary blobs to be loaded into a guest VM, not executables.
> 
> Thomas, maybe simply drop that part about packaging from the commit message.

That works for me as well, thanks!
Laszlo

Patch
diff mbox series

diff --git a/arm/Makefile.common b/arm/Makefile.common
index 1cf9cdc..e62a978 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -73,9 +73,11 @@  FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc) $(libeabi)
 		-Wl,-T,$(SRCDIR)/arm/flat.lds,--build-id=none,-Ttext=$(start_addr) \
 		$(filter %.o, $^) $(FLATLIBS) \
 		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$(@:.elf=.flat)\" -DAUXFLAGS=$(AUXFLAGS)
+	chmod a-x $@
 
 %.flat: %.elf
 	$(OBJCOPY) -O binary $^ $@
+	chmod a-x $@
 
 $(libeabi): $(eabiobjs)
 	$(AR) rcs $@ $^
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 81c5074..af6b9d8 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -54,6 +54,7 @@  FLATLIBS = $(libcflat) $(LIBFDT_archive)
 		-T $(SRCDIR)/powerpc/flat.lds --build-id=none \
 		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
 	$(RM) $(@:.elf=.aux.o)
+	chmod a-x $@
 	@echo -n Checking $@ for unsupported reloc types...
 	@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then	\
 		false;							\
@@ -70,6 +71,7 @@  $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
 $(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian
 $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
 	$(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
+	chmod a-x $@
 
 powerpc_clean: libfdt_clean asm_offsets_clean
 	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
diff --git a/s390x/Makefile b/s390x/Makefile
index 6546a02..6b32f2c 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -53,6 +53,7 @@  FLATLIBS = $(libcflat)
 	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds -Ttext=0x10000 \
 		$(filter %.o, $^) $(FLATLIBS) $(@:.elf=.aux.o)
 	$(RM) $(@:.elf=.aux.o)
+	chmod a-x $@
 
 arch_clean: asm_offsets_clean
 	$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/.*.d lib/s390x/.*.d
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 7dcf8c2..0d64284 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -42,9 +42,11 @@  FLATLIBS = lib/libcflat.a $(libgcc)
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
 	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
 		$(filter %.o, $^) $(FLATLIBS)
+	chmod a-x $@
 
 %.flat: %.elf
 	$(OBJCOPY) -O elf32-i386 $^ $@
+	chmod a-x $@
 
 tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
                $(TEST_DIR)/smptest.flat  $(TEST_DIR)/port80.flat \