[Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string arrays from bootmap header to .c file

[Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string arrays from bootmap header to .c file

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string arrays from bootmap header to .c file

[Christian Borntraeger]

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 {
> 

Re: [Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string arrays from bootmap header to .c file

[Cornelia Huck]

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).

Re: [Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string arrays from bootmap header to .c file

[Eric Blake]

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

Re: [Qemu-devel] [PATCH v2] pc-bios/s390-ccw: Move string arrays from bootmap header to .c file

[Thomas Huth]

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