[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [qemu-s390x] [Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string a

From: Eric Blake
Subject: Re: [qemu-s390x] [Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string arrays from bootmap header to .c file
Date: Wed, 7 Mar 2018 17:11:34 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

On 03/06/2018 12:18 AM, Thomas Huth wrote:
bootmap.h can currently only be included once - otherwise the linker
complains about multiple definitions of the "magic" strings.

My first thought when reading that was "Huh? bootmap.h has a proper[*] double-inclusion header guard, and therefore a second #include "bootmap.h" is a no-op - so how can including the header more than once cause a linker complaint?"

[*] Well, proper if you overlook the fact that the name _PC_BIOS_S390_CCW_BOOTMAP_H starts with a leading underscore followed by uppercase, and is therefore violating namespace safety rules, as it could collide with a symbol reserved for the implementation

It's a
bad style to define string arrays in header files, so let's better
move these to the bootmap.c file instead where they are used.

But I finally figured out what you really meant: if more than one .c file each include the header (and not my initial reading of a single .c file including the header more than once), then since the header was declaring non-static top-level variables, that does indeed cause linker errors.

Signed-off-by: Thomas Huth <address@hidden>
  - Removed duplicated vol_desc_magic (copy-n-paste error)

  pc-bios/s390-ccw/bootmap.c | 20 ++++++++++++++++++++
  pc-bios/s390-ccw/bootmap.h | 19 -------------------
  2 files changed, 20 insertions(+), 19 deletions(-)

Your change is fine (moving the declaration into the one .c file that needs them), so no need to change this, but...

+++ b/pc-bios/s390-ccw/bootmap.h
@@ -375,9 +375,6 @@ static inline void read_iso_boot_image(uint32_t 
block_offset, void *load_addr,
                 "Failed to read boot image!");
-const uint8_t el_torito_magic[] = "EL TORITO SPECIFICATION"
-                                  "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";

...would adding 'static' here also solved the linker error (at the risk of possibly causing a compiler warning/error about unused variable)?

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]