| Message ID | 20240712075834.10605-1-ceggers@arri.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | parser: libconfig: allow implicit conversion from INT to INT64 | expand |
Hi Christian, On 12.07.24 09:58, Christian Eggers wrote: > Since 1db0aefe57de ("Enforce type check in sw-description"), > GET_FIELD_INT64() does not assign a config value anymore if the type > detected by libconfig differs. This type check needs to be slightly > relaxed to allow assignment of parsed INT32 values when a INT64 is > expected. Otherwise this would require conversion of the sw-description > in the .swu files which breaks compatiblity with existing update files. > > Link: https://groups.google.com/g/swupdate/c/UeALEHCAusQ > Signed-off-by: Christian Eggers <ceggers@arri.de> > --- > corelib/parsing_library_libconfig.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c > index ddb79f6fb152..727697448ac4 100644 > --- a/corelib/parsing_library_libconfig.c > +++ b/corelib/parsing_library_libconfig.c > @@ -42,8 +42,13 @@ static unsigned int map_field_type(field_type_t type) > static void get_value_libconfig(const config_setting_t *e, void *dest, field_type_t expected_type) > { > int type = config_setting_type(e); > - if (type != map_field_type(expected_type)) > - return; > + if (type != map_field_type(expected_type)) { > + /* Only allow implicit conversion from INT to INT64 */ > + if (type == CONFIG_TYPE_INT && expected_type == TYPE_INT64) > + type = CONFIG_TYPE_INT64; > + else > + return; > + } There is no conversion, the type is forced to be INT64 and just solves your use case. It will be plausible to call GET_FIELD_INT in the code, and a user has written in sw-description something like attribute = 1234L; If this should be solved, it should be complete in both directions. Best regards, Stefano
Hi Stefano, On Friday, 12 July 2024, 10:11:16 CEST, Stefano Babic wrote: > Hi Christian, > > On 12.07.24 09:58, Christian Eggers wrote: > > Since 1db0aefe57de ("Enforce type check in sw-description"), > > GET_FIELD_INT64() does not assign a config value anymore if the type > > detected by libconfig differs. This type check needs to be slightly > > relaxed to allow assignment of parsed INT32 values when a INT64 is > > expected. Otherwise this would require conversion of the sw-description > > in the .swu files which breaks compatiblity with existing update files. > > > > Link: https://groups.google.com/g/swupdate/c/UeALEHCAusQ > > Signed-off-by: Christian Eggers <ceggers@arri.de> > > --- > > corelib/parsing_library_libconfig.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c > > index ddb79f6fb152..727697448ac4 100644 > > --- a/corelib/parsing_library_libconfig.c > > +++ b/corelib/parsing_library_libconfig.c > > @@ -42,8 +42,13 @@ static unsigned int map_field_type(field_type_t type) > > static void get_value_libconfig(const config_setting_t *e, void *dest, field_type_t expected_type) > > { > > int type = config_setting_type(e); > > - if (type != map_field_type(expected_type)) > > - return; > > + if (type != map_field_type(expected_type)) { > > + /* Only allow implicit conversion from INT to INT64 */ > > + if (type == CONFIG_TYPE_INT && expected_type == TYPE_INT64) > > + type = CONFIG_TYPE_INT64; > > + else > > + return; > > + } > > There is no conversion, the type is forced to be INT64 and just solves > your use case. It will be plausible to call GET_FIELD_INT in the code, > and a user has written in sw-description something like > attribute = 1234L; > > If this should be solved, it should be complete in both directions. This would obviously weaken the type safety requirements you just introduced. Maybe this could be postponed until somebody actually complains? How should INT64 values which don't fit into INT32 be handled? I also think that it is problematic that get_value_libconfig() simply does nothing if there is a type mismatch. I would prefer printing at least a warning that there was a problem parsing the sw-description. Otherwise the problem hits in somewhere else (e.g. deleting UBI volumes in my case) and requires using a debugger for finding the source. Best regards, Christian
On 12.07.24 13:12, Christian Eggers wrote: > Hi Stefano, > > On Friday, 12 July 2024, 10:11:16 CEST, Stefano Babic wrote: >> Hi Christian, >> >> On 12.07.24 09:58, Christian Eggers wrote: >>> Since 1db0aefe57de ("Enforce type check in sw-description"), >>> GET_FIELD_INT64() does not assign a config value anymore if the type >>> detected by libconfig differs. This type check needs to be slightly >>> relaxed to allow assignment of parsed INT32 values when a INT64 is >>> expected. Otherwise this would require conversion of the sw-description >>> in the .swu files which breaks compatiblity with existing update files. >>> >>> Link: https://groups.google.com/g/swupdate/c/UeALEHCAusQ >>> Signed-off-by: Christian Eggers <ceggers@arri.de> >>> --- >>> corelib/parsing_library_libconfig.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c >>> index ddb79f6fb152..727697448ac4 100644 >>> --- a/corelib/parsing_library_libconfig.c >>> +++ b/corelib/parsing_library_libconfig.c >>> @@ -42,8 +42,13 @@ static unsigned int map_field_type(field_type_t type) >>> static void get_value_libconfig(const config_setting_t *e, void *dest, field_type_t expected_type) >>> { >>> int type = config_setting_type(e); >>> - if (type != map_field_type(expected_type)) >>> - return; >>> + if (type != map_field_type(expected_type)) { >>> + /* Only allow implicit conversion from INT to INT64 */ >>> + if (type == CONFIG_TYPE_INT && expected_type == TYPE_INT64) >>> + type = CONFIG_TYPE_INT64; >>> + else >>> + return; >>> + } >> >> There is no conversion, the type is forced to be INT64 and just solves >> your use case. It will be plausible to call GET_FIELD_INT in the code, >> and a user has written in sw-description something like >> attribute = 1234L; >> >> If this should be solved, it should be complete in both directions. > > This would obviously weaken the type safety requirements you just introduced. For int and int64, yes. But I do not see how we can have both and maintain compatibility. Or we say well, old SWUs must be updated and resigned. > Maybe this could be postponed until somebody actually complains? In my experience, this means never and it remains unsolved until there is another breakage and then a new work-around is searched. > How should INT64 values which don't fit into INT32 be handled? The master is SWUpdate internal type, and it decides which is the type to be internal stored. The case can be detected because libconfig is reporting the type. In such case, a WARN should be raised and the parser does its best. But it can be also detected if INT64 fit or not, and in the second case an ERROR could be raised and parser stops. > > I also think that it is problematic that get_value_libconfig() simply > does nothing if there is a type mismatch. I would prefer printing at > least a warning that there was a problem parsing the sw-description. Agree, a warning should be raised in mismatch. > Otherwise the problem hits in somewhere else (e.g. deleting UBI volumes > in my case) and requires using a debugger for finding the source. > Best regards, Stefano
diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c index ddb79f6fb152..727697448ac4 100644 --- a/corelib/parsing_library_libconfig.c +++ b/corelib/parsing_library_libconfig.c @@ -42,8 +42,13 @@ static unsigned int map_field_type(field_type_t type) static void get_value_libconfig(const config_setting_t *e, void *dest, field_type_t expected_type) { int type = config_setting_type(e); - if (type != map_field_type(expected_type)) - return; + if (type != map_field_type(expected_type)) { + /* Only allow implicit conversion from INT to INT64 */ + if (type == CONFIG_TYPE_INT && expected_type == TYPE_INT64) + type = CONFIG_TYPE_INT64; + else + return; + } switch (type) { case CONFIG_TYPE_INT: *(int *)dest = config_setting_get_int(e);
Since 1db0aefe57de ("Enforce type check in sw-description"), GET_FIELD_INT64() does not assign a config value anymore if the type detected by libconfig differs. This type check needs to be slightly relaxed to allow assignment of parsed INT32 values when a INT64 is expected. Otherwise this would require conversion of the sw-description in the .swu files which breaks compatiblity with existing update files. Link: https://groups.google.com/g/swupdate/c/UeALEHCAusQ Signed-off-by: Christian Eggers <ceggers@arri.de> --- corelib/parsing_library_libconfig.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)