"hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN,
use that instead of the magic '8' value. Use a char type for
char buffer.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/s390x/ccw-device.h | 5 +++--
hw/s390x/ipl.h | 2 +-
include/hw/qdev-properties-system.h | 2 +-
include/hw/s390x/s390-virtio-ccw.h | 3 ++-
hw/core/qdev-properties-system.c | 8 +++++---
hw/s390x/ipl.c | 6 +++---
hw/scsi/scsi-disk.c | 2 +-
7 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
index 4439feb140..94a9b35714 100644
--- a/hw/s390x/ccw-device.h
+++ b/hw/s390x/ccw-device.h
@@ -15,6 +15,7 @@
#include "hw/qdev-core.h"
#include "hw/s390x/css.h"
#include "hw/s390x/css-bridge.h"
+#include "hw/s390x/ipl/qipl.h"
struct CcwDevice {
DeviceState parent_obj;
@@ -27,7 +28,7 @@ struct CcwDevice {
/* The actual busid of the virtual subchannel. */
CssDevId subch_id;
/* If set, use this loadparm value when device is boot target */
- uint8_t loadparm[8];
+ char loadparm[LOADPARM_LEN];
};
typedef struct CcwDevice CcwDevice;
@@ -54,6 +55,6 @@ OBJECT_DECLARE_TYPE(CcwDevice, CCWDeviceClass, CCW_DEVICE)
extern const PropertyInfo ccw_loadparm;
#define DEFINE_PROP_CCW_LOADPARM(_n, _s, _f) \
- DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(uint8_t[8]))
+ DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(char[LOADPARM_LEN]))
#endif
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index d7d0b7bfd2..014b530ad2 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -23,7 +23,7 @@
#define MAX_BOOT_DEVS 8 /* Max number of devices that may have a bootindex */
void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
-void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
+void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp);
void s390_rebuild_iplb(uint16_t index, IplParameterBlock *iplb);
void s390_ipl_update_diag308(IplParameterBlock *iplb);
int s390_ipl_prepare_pv_header(Error **errp);
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 7ec37f6316..844af7a200 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -3,7 +3,7 @@
#include "hw/qdev-properties.h"
-bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
+bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str,
Error **errp);
extern const PropertyInfo qdev_prop_chr;
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 996864a34e..9d4e00b0c7 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -14,6 +14,7 @@
#include "hw/boards.h"
#include "qom/object.h"
#include "hw/s390x/sclp.h"
+#include "hw/s390x/ipl/qipl.h"
#define TYPE_S390_CCW_MACHINE "s390-ccw-machine"
@@ -28,7 +29,7 @@ struct S390CcwMachineState {
bool aes_key_wrap;
bool dea_key_wrap;
bool pv;
- uint8_t loadparm[8];
+ char loadparm[LOADPARM_LEN];
SCLPDevice *sclp;
};
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index a61c5ee6dd..e8e9cd8e04 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -35,6 +35,7 @@
#include "hw/pci/pci.h"
#include "hw/pci/pcie.h"
#include "hw/i386/x86.h"
+#include "hw/s390x/ipl/qipl.h"
#include "util/block-helpers.h"
static bool check_prop_still_unset(Object *obj, const char *name,
@@ -58,14 +59,15 @@ static bool check_prop_still_unset(Object *obj, const char *name,
return false;
}
-bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
+bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str,
Error **errp)
{
int i, len;
len = strlen(str);
- if (len > 8) {
- error_setg(errp, "'loadparm' can only contain up to 8 characters");
+ if (len > LOADPARM_LEN) {
+ error_setg(errp, "'loadparm' can only contain up to %u characters",
+ LOADPARM_LEN);
return false;
}
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 30734661ad..6fd3774c7d 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -416,7 +416,7 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
return chain_addr;
}
-void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
+void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp)
{
/* Initialize the loadparm with spaces */
memset(loadparm, ' ', LOADPARM_LEN);
@@ -439,8 +439,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
CcwDevice *ccw_dev = NULL;
SCSIDevice *sd;
int devtype;
- uint8_t *lp;
- g_autofree void *scsi_lp = NULL;
+ char *lp;
+ g_autofree char *scsi_lp = NULL;
/*
* Currently allow IPL only from CCW devices.
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8e553487d5..96a09fe170 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -3145,7 +3145,7 @@ static char *scsi_property_get_loadparm(Object *obj, Error **errp)
static void scsi_property_set_loadparm(Object *obj, const char *value,
Error **errp)
{
- void *lp_str;
+ char *lp_str;
if (object_property_get_int(obj, "bootindex", NULL) < 0) {
error_setg(errp, "'loadparm' is only valid for boot devices");
--
2.45.2
ping for this single cleanup patch?
On 20/11/24 09:52, Philippe Mathieu-Daudé wrote:
> "hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN,
> use that instead of the magic '8' value. Use a char type for
> char buffer.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/s390x/ccw-device.h | 5 +++--
> hw/s390x/ipl.h | 2 +-
> include/hw/qdev-properties-system.h | 2 +-
> include/hw/s390x/s390-virtio-ccw.h | 3 ++-
> hw/core/qdev-properties-system.c | 8 +++++---
> hw/s390x/ipl.c | 6 +++---
> hw/scsi/scsi-disk.c | 2 +-
> 7 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
> index 4439feb140..94a9b35714 100644
> --- a/hw/s390x/ccw-device.h
> +++ b/hw/s390x/ccw-device.h
> @@ -15,6 +15,7 @@
> #include "hw/qdev-core.h"
> #include "hw/s390x/css.h"
> #include "hw/s390x/css-bridge.h"
> +#include "hw/s390x/ipl/qipl.h"
>
> struct CcwDevice {
> DeviceState parent_obj;
> @@ -27,7 +28,7 @@ struct CcwDevice {
> /* The actual busid of the virtual subchannel. */
> CssDevId subch_id;
> /* If set, use this loadparm value when device is boot target */
> - uint8_t loadparm[8];
> + char loadparm[LOADPARM_LEN];
> };
> typedef struct CcwDevice CcwDevice;
>
> @@ -54,6 +55,6 @@ OBJECT_DECLARE_TYPE(CcwDevice, CCWDeviceClass, CCW_DEVICE)
> extern const PropertyInfo ccw_loadparm;
>
> #define DEFINE_PROP_CCW_LOADPARM(_n, _s, _f) \
> - DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(uint8_t[8]))
> + DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(char[LOADPARM_LEN]))
>
> #endif
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index d7d0b7bfd2..014b530ad2 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -23,7 +23,7 @@
> #define MAX_BOOT_DEVS 8 /* Max number of devices that may have a bootindex */
>
> void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
> -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
> +void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp);
> void s390_rebuild_iplb(uint16_t index, IplParameterBlock *iplb);
> void s390_ipl_update_diag308(IplParameterBlock *iplb);
> int s390_ipl_prepare_pv_header(Error **errp);
> diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
> index 7ec37f6316..844af7a200 100644
> --- a/include/hw/qdev-properties-system.h
> +++ b/include/hw/qdev-properties-system.h
> @@ -3,7 +3,7 @@
>
> #include "hw/qdev-properties.h"
>
> -bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
> +bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str,
> Error **errp);
>
> extern const PropertyInfo qdev_prop_chr;
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 996864a34e..9d4e00b0c7 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -14,6 +14,7 @@
> #include "hw/boards.h"
> #include "qom/object.h"
> #include "hw/s390x/sclp.h"
> +#include "hw/s390x/ipl/qipl.h"
>
> #define TYPE_S390_CCW_MACHINE "s390-ccw-machine"
>
> @@ -28,7 +29,7 @@ struct S390CcwMachineState {
> bool aes_key_wrap;
> bool dea_key_wrap;
> bool pv;
> - uint8_t loadparm[8];
> + char loadparm[LOADPARM_LEN];
>
> SCLPDevice *sclp;
> };
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index a61c5ee6dd..e8e9cd8e04 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -35,6 +35,7 @@
> #include "hw/pci/pci.h"
> #include "hw/pci/pcie.h"
> #include "hw/i386/x86.h"
> +#include "hw/s390x/ipl/qipl.h"
> #include "util/block-helpers.h"
>
> static bool check_prop_still_unset(Object *obj, const char *name,
> @@ -58,14 +59,15 @@ static bool check_prop_still_unset(Object *obj, const char *name,
> return false;
> }
>
> -bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
> +bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str,
> Error **errp)
> {
> int i, len;
>
> len = strlen(str);
> - if (len > 8) {
> - error_setg(errp, "'loadparm' can only contain up to 8 characters");
> + if (len > LOADPARM_LEN) {
> + error_setg(errp, "'loadparm' can only contain up to %u characters",
> + LOADPARM_LEN);
> return false;
> }
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 30734661ad..6fd3774c7d 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -416,7 +416,7 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
> return chain_addr;
> }
>
> -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp)
> +void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp)
> {
> /* Initialize the loadparm with spaces */
> memset(loadparm, ' ', LOADPARM_LEN);
> @@ -439,8 +439,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
> CcwDevice *ccw_dev = NULL;
> SCSIDevice *sd;
> int devtype;
> - uint8_t *lp;
> - g_autofree void *scsi_lp = NULL;
> + char *lp;
> + g_autofree char *scsi_lp = NULL;
>
> /*
> * Currently allow IPL only from CCW devices.
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 8e553487d5..96a09fe170 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -3145,7 +3145,7 @@ static char *scsi_property_get_loadparm(Object *obj, Error **errp)
> static void scsi_property_set_loadparm(Object *obj, const char *value,
> Error **errp)
> {
> - void *lp_str;
> + char *lp_str;
>
> if (object_property_get_int(obj, "bootindex", NULL) < 0) {
> error_setg(errp, "'loadparm' is only valid for boot devices");
On 30/12/2024 18.02, Philippe Mathieu-Daudé wrote:
> ping for this single cleanup patch?
>
> On 20/11/24 09:52, Philippe Mathieu-Daudé wrote:
>> "hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN,
>> use that instead of the magic '8' value. Use a char type for
>> char buffer.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/s390x/ccw-device.h | 5 +++--
>> hw/s390x/ipl.h | 2 +-
>> include/hw/qdev-properties-system.h | 2 +-
>> include/hw/s390x/s390-virtio-ccw.h | 3 ++-
>> hw/core/qdev-properties-system.c | 8 +++++---
>> hw/s390x/ipl.c | 6 +++---
>> hw/scsi/scsi-disk.c | 2 +-
>> 7 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
>> index 4439feb140..94a9b35714 100644
>> --- a/hw/s390x/ccw-device.h
>> +++ b/hw/s390x/ccw-device.h
>> @@ -15,6 +15,7 @@
>> #include "hw/qdev-core.h"
>> #include "hw/s390x/css.h"
>> #include "hw/s390x/css-bridge.h"
>> +#include "hw/s390x/ipl/qipl.h"
>> struct CcwDevice {
>> DeviceState parent_obj;
>> @@ -27,7 +28,7 @@ struct CcwDevice {
>> /* The actual busid of the virtual subchannel. */
>> CssDevId subch_id;
>> /* If set, use this loadparm value when device is boot target */
>> - uint8_t loadparm[8];
>> + char loadparm[LOADPARM_LEN];
I'm not sure whether changing the type is such a good idea ... some spots
that handle loadparm include EBCDIC encoding, and the unsigned type might
have been used here for a reason? I'd better keep uint8_t unless you've
double-checked all combinations...
Thomas
On 3/1/25 18:17, Thomas Huth wrote:
> On 30/12/2024 18.02, Philippe Mathieu-Daudé wrote:
>> ping for this single cleanup patch?
>>
>> On 20/11/24 09:52, Philippe Mathieu-Daudé wrote:
>>> "hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN,
>>> use that instead of the magic '8' value. Use a char type for
>>> char buffer.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/s390x/ccw-device.h | 5 +++--
>>> hw/s390x/ipl.h | 2 +-
>>> include/hw/qdev-properties-system.h | 2 +-
>>> include/hw/s390x/s390-virtio-ccw.h | 3 ++-
>>> hw/core/qdev-properties-system.c | 8 +++++---
>>> hw/s390x/ipl.c | 6 +++---
>>> hw/scsi/scsi-disk.c | 2 +-
>>> 7 files changed, 16 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h
>>> index 4439feb140..94a9b35714 100644
>>> --- a/hw/s390x/ccw-device.h
>>> +++ b/hw/s390x/ccw-device.h
>>> @@ -15,6 +15,7 @@
>>> #include "hw/qdev-core.h"
>>> #include "hw/s390x/css.h"
>>> #include "hw/s390x/css-bridge.h"
>>> +#include "hw/s390x/ipl/qipl.h"
>>> struct CcwDevice {
>>> DeviceState parent_obj;
>>> @@ -27,7 +28,7 @@ struct CcwDevice {
>>> /* The actual busid of the virtual subchannel. */
>>> CssDevId subch_id;
>>> /* If set, use this loadparm value when device is boot target */
>>> - uint8_t loadparm[8];
>>> + char loadparm[LOADPARM_LEN];
>
> I'm not sure whether changing the type is such a good idea ... some
> spots that handle loadparm include EBCDIC encoding, and the unsigned
> type might have been used here for a reason? I'd better keep uint8_t
> unless you've double-checked all combinations...
OK, no problem, thanks for checking!
Regards,
Phil.
© 2016 - 2026 Red Hat, Inc.