On Fri, Jun 2, 2017 at 6:00 PM Marc-André Lureau <marcandre.lur...@gmail.com> wrote:
> On Fri, Jun 2, 2017 at 5:55 PM Markus Armbruster <arm...@redhat.com> > wrote: > >> Marc-André Lureau <marcandre.lur...@redhat.com> writes: >> >> > Add and use unsigned type for various properties. >> > >> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> The commit message is a bit misleading. We don't "add unsigned >> properties", we clean up the property code to avoid type casts between >> signed and unsigned. >> > > That in itself is imho a good reason for the cleanup :) > There might be other reason, as if a value is stored as negative QNum/i64 after cast, and later retrieve with get_uint(), it will fail. >> I'm not sure that's worth the churn by itself. But perhaps it helps >> later in this series. If yes, can you give me a hint? >> > > Churn is quite minimal though. > > >> >> > --- >> > include/hw/qdev-core.h | 1 + >> > include/hw/qdev-properties.h | 51 >> +++++++++++++++++++++++++++----------------- >> > hw/core/qdev-properties.c | 23 ++++++++++++-------- >> > 3 files changed, 46 insertions(+), 29 deletions(-) >> > >> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> > index 784971b8d8..9d7c1c0e9b 100644 >> > --- a/include/hw/qdev-core.h >> > +++ b/include/hw/qdev-core.h >> > @@ -228,6 +228,7 @@ struct Property { >> > uint8_t bitnr; >> > union { >> > int64_t i; >> > + uint64_t u; >> > } defval; >> > int arrayoffset; >> > PropertyInfo *arrayinfo; >> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h >> > index 6663a547ea..16d3ada8df 100644 >> > --- a/include/hw/qdev-properties.h >> > +++ b/include/hw/qdev-properties.h >> > @@ -37,6 +37,7 @@ extern PropertyInfo qdev_prop_arraylen; >> > .offset = offsetof(_state, _field) \ >> > + type_check(_type, typeof_field(_state, _field)), \ >> > } >> > + >> > #define DEFINE_PROP_INT(_name, _state, _field, _defval, _prop, _type) >> { \ >> > .name = (_name), >> \ >> > .info = &(_prop), >> \ >> > @@ -44,29 +45,39 @@ extern PropertyInfo qdev_prop_arraylen; >> > + type_check(_type,typeof_field(_state, _field)), >> \ >> > .defval.i = (_type)_defval, >> \ >> > } >> > -#define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { \ >> > - .name = (_name), \ >> > - .info = &(qdev_prop_bit), \ >> > - .bitnr = (_bit), \ >> > - .offset = offsetof(_state, _field) \ >> > - + type_check(uint32_t,typeof_field(_state, _field)), \ >> > - .defval.i = (bool)_defval, \ >> > + >> > +#define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) { >> \ >> > + .name = (_name), >> \ >> > + .info = &(qdev_prop_bit), >> \ >> > + .bitnr = (_bit), >> \ >> > + .offset = offsetof(_state, _field) >> \ >> > + + type_check(uint32_t, typeof_field(_state, _field)), >> \ >> > + .defval.u = (bool)_defval, >> \ >> > } >> >> Let's keep the backslashes where they are to reduce churn. >> >> > + >> > +#define DEFINE_PROP_UINT(_name, _state, _field, _defval, _prop, _type) >> { \ >> > + .name = (_name), >> \ >> > + .info = &(_prop), >> \ >> > + .offset = offsetof(_state, _field) >> \ >> > + + type_check(_type, typeof_field(_state, _field)), >> \ >> > + .defval.u = (_type)_defval, >> \ >> > + } >> > + >> > #define DEFINE_PROP_BIT64(_name, _state, _field, _bit, _defval) { >> \ >> > .name = (_name), >> \ >> > .info = &(qdev_prop_bit64), >> \ >> > .bitnr = (_bit), >> \ >> > .offset = offsetof(_state, _field) >> \ >> > + type_check(uint64_t, typeof_field(_state, _field)), >> \ >> > - .defval.i = (bool)_defval, >> \ >> > + .defval.u = (bool)_defval, >> \ >> > } >> > >> > -#define DEFINE_PROP_BOOL(_name, _state, _field, _defval) { \ >> > - .name = (_name), \ >> > - .info = &(qdev_prop_bool), \ >> > - .offset = offsetof(_state, _field) \ >> > - + type_check(bool, typeof_field(_state, _field)), \ >> > - .defval.i = (bool)_defval, \ >> > +#define DEFINE_PROP_BOOL(_name, _state, _field, _defval) { \ >> > + .name = (_name), \ >> > + .info = &(qdev_prop_bool), \ >> > + .offset = offsetof(_state, _field) \ >> > + + type_check(bool, typeof_field(_state, _field)), \ >> > + .defval.u = (bool)_defval, \ >> > } >> >> Likewise. >> >> > >> > #define PROP_ARRAY_LEN_PREFIX "len-" >> > @@ -107,17 +118,17 @@ extern PropertyInfo qdev_prop_arraylen; >> > } >> > >> > #define DEFINE_PROP_UINT8(_n, _s, _f, _d) \ >> > - DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t) >> > + DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint8, uint8_t) >> > #define DEFINE_PROP_UINT16(_n, _s, _f, _d) \ >> > - DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint16, uint16_t) >> > + DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint16, uint16_t) >> > #define DEFINE_PROP_UINT32(_n, _s, _f, _d) \ >> > - DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint32, uint32_t) >> > + DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint32, uint32_t) >> > #define DEFINE_PROP_INT32(_n, _s, _f, _d) \ >> > DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_int32, int32_t) >> > #define DEFINE_PROP_UINT64(_n, _s, _f, _d) \ >> > - DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t) >> > + DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_uint64, uint64_t) >> > #define DEFINE_PROP_SIZE(_n, _s, _f, _d) \ >> > - DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_size, uint64_t) >> > + DEFINE_PROP_UINT(_n, _s, _f, _d, qdev_prop_size, uint64_t) >> > #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d) \ >> > DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t) >> > >> > @@ -163,7 +174,7 @@ extern PropertyInfo qdev_prop_arraylen; >> > #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \ >> > DEFINE_PROP_INT(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int) >> > #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \ >> > - DEFINE_PROP_INT(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t) >> > + DEFINE_PROP_UINT(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t) >> > #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \ >> > DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, >> PCIHostDeviceAddress) >> > >> > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> > index 952cda7758..92fb43cb0d 100644 >> > --- a/hw/core/qdev-properties.c >> > +++ b/hw/core/qdev-properties.c >> > @@ -71,7 +71,7 @@ static void set_enum(Object *obj, Visitor *v, const >> char *name, void *opaque, >> > >> > static void set_default_value_enum(Object *obj, const Property *prop) >> > { >> > - object_property_set_str(obj, >> prop->info->enum_table[prop->defval.i], >> > + object_property_set_str(obj, >> prop->info->enum_table[prop->defval.u], >> > prop->name, &error_abort); >> > } >> > >> > @@ -128,7 +128,7 @@ static void prop_set_bit(Object *obj, Visitor *v, >> const char *name, >> > >> > static void set_default_value_bool(Object *obj, const Property *prop) >> > { >> > - object_property_set_bool(obj, prop->defval.i, prop->name, >> &error_abort); >> > + object_property_set_bool(obj, prop->defval.u, prop->name, >> &error_abort); >> > } >> > >> > PropertyInfo qdev_prop_bit = { >> > @@ -264,11 +264,16 @@ static void set_default_value_int(Object *obj, >> const Property *prop) >> > object_property_set_int(obj, prop->defval.i, prop->name, >> &error_abort); >> > } >> > >> > +static void set_default_value_uint(Object *obj, const Property *prop) >> > +{ >> > + object_property_set_uint(obj, prop->defval.u, prop->name, >> &error_abort); >> > +} >> > + >> > PropertyInfo qdev_prop_uint8 = { >> > .name = "uint8", >> > .get = get_uint8, >> > .set = set_uint8, >> > - .set_default_value = set_default_value_int, >> > + .set_default_value = set_default_value_uint, >> > }; >> > >> > /* --- 16bit integer --- */ >> > @@ -302,7 +307,7 @@ PropertyInfo qdev_prop_uint16 = { >> > .name = "uint16", >> > .get = get_uint16, >> > .set = set_uint16, >> > - .set_default_value = set_default_value_int, >> > + .set_default_value = set_default_value_uint, >> > }; >> > >> > /* --- 32bit integer --- */ >> > @@ -361,7 +366,7 @@ PropertyInfo qdev_prop_uint32 = { >> > .name = "uint32", >> > .get = get_uint32, >> > .set = set_uint32, >> > - .set_default_value = set_default_value_int, >> > + .set_default_value = set_default_value_uint, >> > }; >> > >> > PropertyInfo qdev_prop_int32 = { >> > @@ -402,7 +407,7 @@ PropertyInfo qdev_prop_uint64 = { >> > .name = "uint64", >> > .get = get_uint64, >> > .set = set_uint64, >> > - .set_default_value = set_default_value_int, >> > + .set_default_value = set_default_value_uint, >> > }; >> > >> > /* --- string --- */ >> > @@ -725,7 +730,7 @@ PropertyInfo qdev_prop_blocksize = { >> > .description = "A power of two between 512 and 32768", >> > .get = get_uint16, >> > .set = set_blocksize, >> > - .set_default_value = set_default_value_int, >> > + .set_default_value = set_default_value_uint, >> > }; >> > >> > /* --- pci host address --- */ >> > @@ -948,7 +953,7 @@ PropertyInfo qdev_prop_arraylen = { >> > .name = "uint32", >> > .get = get_uint32, >> > .set = set_prop_arraylen, >> > - .set_default_value = set_default_value_int, >> > + .set_default_value = set_default_value_uint, >> > }; >> > >> > /* --- public helpers --- */ >> > @@ -1185,5 +1190,5 @@ PropertyInfo qdev_prop_size = { >> > .name = "size", >> > .get = get_size, >> > .set = set_size, >> > - .set_default_value = set_default_value_int, >> > + .set_default_value = set_default_value_uint, >> > }; >> >> -- > Marc-André Lureau > -- Marc-André Lureau