pc-bios/s390-ccw/bootmap.c | 20 ++++++++++++++++++++ pc-bios/s390-ccw/bootmap.h | 19 ------------------- 2 files changed, 20 insertions(+), 19 deletions(-)
bootmap.h can currently only be included once - otherwise the linker
complains about multiple definitions of the "magic" strings. 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.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
v2:
- 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(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 29bfd8c..fc2a9fe 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -37,6 +37,26 @@ typedef struct ResetInfo {
static ResetInfo save;
+const uint8_t el_torito_magic[] = "EL TORITO SPECIFICATION"
+ "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";
+
+/*
+ * Match two CCWs located after PSW and eight filler bytes.
+ * From libmagic and arch/s390/kernel/head.S.
+ */
+const uint8_t linux_s390_magic[] = "\x02\x00\x00\x18\x60\x00\x00\x50\x02\x00"
+ "\x00\x68\x60\x00\x00\x50\x40\x40\x40\x40"
+ "\x40\x40\x40\x40";
+
+static inline bool is_iso_vd_valid(IsoVolDesc *vd)
+{
+ const uint8_t vol_desc_magic[] = "CD001";
+
+ return !memcmp(&vd->ident[0], vol_desc_magic, 5) &&
+ vd->version == 0x1 &&
+ vd->type <= VOL_DESC_TYPE_PARTITION;
+}
+
static void jump_to_IPL_2(void)
{
ResetInfo *current = 0;
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index c636626..07eb600 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ 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";
-
#define ISO9660_MAX_DIR_DEPTH 8
typedef struct IsoDirHdr {
@@ -430,20 +427,12 @@ typedef struct IsoVolDesc {
} vd;
} __attribute__((packed)) IsoVolDesc;
-const uint8_t vol_desc_magic[] = "CD001";
#define VOL_DESC_TYPE_BOOT 0
#define VOL_DESC_TYPE_PRIMARY 1
#define VOL_DESC_TYPE_SUPPLEMENT 2
#define VOL_DESC_TYPE_PARTITION 3
#define VOL_DESC_TERMINATOR 255
-static inline bool is_iso_vd_valid(IsoVolDesc *vd)
-{
- return !memcmp(&vd->ident[0], vol_desc_magic, 5) &&
- vd->version == 0x1 &&
- vd->type <= VOL_DESC_TYPE_PARTITION;
-}
-
typedef struct IsoBcValid {
uint8_t platform_id;
uint16_t reserved;
@@ -468,14 +457,6 @@ typedef struct IsoBcHdr {
uint8_t id[28];
} __attribute__((packed)) IsoBcHdr;
-/*
- * Match two CCWs located after PSW and eight filler bytes.
- * From libmagic and arch/s390/kernel/head.S.
- */
-const uint8_t linux_s390_magic[] = "\x02\x00\x00\x18\x60\x00\x00\x50\x02\x00"
- "\x00\x68\x60\x00\x00\x50\x40\x40\x40\x40"
- "\x40\x40\x40\x40";
-
typedef struct IsoBcEntry {
uint8_t id;
union {
--
1.8.3.1
On 03/06/2018 07:18 AM, Thomas Huth wrote:
> bootmap.h can currently only be included once - otherwise the linker
> complains about multiple definitions of the "magic" strings. 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.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v2:
> - 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(-)
>
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 29bfd8c..fc2a9fe 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -37,6 +37,26 @@ typedef struct ResetInfo {
>
> static ResetInfo save;
>
> +const uint8_t el_torito_magic[] = "EL TORITO SPECIFICATION"
> + "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";
> +
> +/*
> + * Match two CCWs located after PSW and eight filler bytes.
> + * From libmagic and arch/s390/kernel/head.S.
> + */
> +const uint8_t linux_s390_magic[] = "\x02\x00\x00\x18\x60\x00\x00\x50\x02\x00"
> + "\x00\x68\x60\x00\x00\x50\x40\x40\x40\x40"
> + "\x40\x40\x40\x40";
> +
> +static inline bool is_iso_vd_valid(IsoVolDesc *vd)
> +{
> + const uint8_t vol_desc_magic[] = "CD001";
> +
> + return !memcmp(&vd->ident[0], vol_desc_magic, 5) &&
> + vd->version == 0x1 &&
> + vd->type <= VOL_DESC_TYPE_PARTITION;
> +}
> +
> static void jump_to_IPL_2(void)
> {
> ResetInfo *current = 0;
> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> index c636626..07eb600 100644
> --- a/pc-bios/s390-ccw/bootmap.h
> +++ 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";
> -
> #define ISO9660_MAX_DIR_DEPTH 8
>
> typedef struct IsoDirHdr {
> @@ -430,20 +427,12 @@ typedef struct IsoVolDesc {
> } vd;
> } __attribute__((packed)) IsoVolDesc;
>
> -const uint8_t vol_desc_magic[] = "CD001";
> #define VOL_DESC_TYPE_BOOT 0
> #define VOL_DESC_TYPE_PRIMARY 1
> #define VOL_DESC_TYPE_SUPPLEMENT 2
> #define VOL_DESC_TYPE_PARTITION 3
> #define VOL_DESC_TERMINATOR 255
>
> -static inline bool is_iso_vd_valid(IsoVolDesc *vd)
> -{
> - return !memcmp(&vd->ident[0], vol_desc_magic, 5) &&
> - vd->version == 0x1 &&
> - vd->type <= VOL_DESC_TYPE_PARTITION;
> -}
> -
> typedef struct IsoBcValid {
> uint8_t platform_id;
> uint16_t reserved;
> @@ -468,14 +457,6 @@ typedef struct IsoBcHdr {
> uint8_t id[28];
> } __attribute__((packed)) IsoBcHdr;
>
> -/*
> - * Match two CCWs located after PSW and eight filler bytes.
> - * From libmagic and arch/s390/kernel/head.S.
> - */
> -const uint8_t linux_s390_magic[] = "\x02\x00\x00\x18\x60\x00\x00\x50\x02\x00"
> - "\x00\x68\x60\x00\x00\x50\x40\x40\x40\x40"
> - "\x40\x40\x40\x40";
> -
> typedef struct IsoBcEntry {
> uint8_t id;
> union {
>
On Tue, 6 Mar 2018 07:18:01 +0100 Thomas Huth <thuth@redhat.com> wrote: > bootmap.h can currently only be included once - otherwise the linker > complains about multiple definitions of the "magic" strings. 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. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > v2: > - 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(-) Thanks, applied (without rebuild as this is no functional change).
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 <thuth@redhat.com> > --- > v2: > - 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
On 08.03.2018 00:11, Eric Blake wrote: > 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?" Sorry if the description was not precise enough ... but I think it's clear if you think about it twice ;-) > [*] 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 Yeah, we've got a couple of these left. I recently added a task to the BiteSizedTasks page to clean those up. > 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)? Yes, we would likely get a warning about unused variable instead, so that's not a real option. Thomas
© 2016 - 2025 Red Hat, Inc.