| Message ID | 20230109135525.877966-1-michael.adler@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fix(cli): always allow setting hardware boardname | expand |
Hi Michael, On 09.01.23 08:55, Michael Adler wrote: > There are two methods for populating struct hw_type: > > 1) using get_hw_revision (in util.c) > 2) using the CLI argument -H > Correct > Method 1) will either parse CONFIG_HW_COMPATIBILITY_FILE or use > /etc/hwrevision if it's not set. > Method 2) is only available if CONFIG_HW_COMPATIBILITY is set. > > This is inconsistent though, mmhh....I do not think so. The switch is just CONFIG_HW_COMPATIBILITY. If this is not set, any value in /etc/hw_revision is not read. You can still set it, but it is not used to check if SW is compatible with the hardware. That means the behavior is consistent and it just depends on CONFIG_HW_COMPATIBILITY. > because if SWUpdate is built without > CONFIG_HW_COMPATIBILITY, then the hardware boardname can still be set > using /etc/hwrevision but not via CLI. I do not think so, there is no check - you can just get the trace "running on ..:", but the check is commented. See check_hw_compatibility() function. > Setting the hw boardname is useful even without checking hw revisions > (parse_hw_compatibility), because it is also used in find_node_and_path > (parser.c) to traverse sw-description and select the correct software. Right, it is useful, but then a hardware-check should be enabled. If I do not want the check, sw-description should not contain any node with board name (we do not want it). Setting a board name into sw-description, but disabling CONFIG_HW_COMPATIBILITY is a sort of hybrid that is unsupported. > > Signed-off-by: Michael Adler <michael.adler@siemens.com> > --- > core/swupdate.c | 9 +-------- > doc/source/swupdate.rst | 4 ++-- > 2 files changed, 3 insertions(+), 10 deletions(-) > > diff --git a/core/swupdate.c b/core/swupdate.c > index 34c9e34..edf563c 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -85,9 +85,7 @@ static struct option long_options[] = { > {"file", required_argument, NULL, 'f'}, > {"get-root", no_argument, NULL, 'g'}, > {"help", no_argument, NULL, 'h'}, > -#ifdef CONFIG_HW_COMPATIBILITY > {"hwrevision", required_argument, NULL, 'H'}, > -#endif > {"image", required_argument, NULL, 'i'}, > #ifdef CONFIG_SIGNED_IMAGES > {"key", required_argument, NULL, 'k'}, > @@ -165,9 +163,7 @@ static void usage(char *programname) > " -o, --output <filename> : saves the incoming stream\n" > " -v, --verbose : be verbose, set maximum loglevel\n" > " --version : print SWUpdate version and exit\n" > -#ifdef CONFIG_HW_COMPATIBILITY > " -H, --hwrevision <board>:<rev> : Set hardware revision\n" > -#endif > " -c, --check : check image and exit, use with -i <filename>\n" > " -h, --help : print this help and exit\n" > ); > @@ -447,7 +443,7 @@ int main(int argc, char **argv) > #endif > memset(main_options, 0, sizeof(main_options)); > memset(image_url, 0, sizeof(image_url)); > - strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:N:R:MmB:"); > + strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:N:R:MmB:H:"); > #ifdef CONFIG_MTD > strcat(main_options, "b:"); > #endif > @@ -460,9 +456,6 @@ int main(int argc, char **argv) > #ifdef CONFIG_WEBSERVER > strcat(main_options, "w:"); > #endif > -#ifdef CONFIG_HW_COMPATIBILITY > - strcat(main_options, "H:"); > -#endif > #ifdef CONFIG_SIGNED_IMAGES > strcat(main_options, "k:"); > public_key_mandatory = 1; > diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst > index 802b16a..4fcca22 100644 > --- a/doc/source/swupdate.rst > +++ b/doc/source/swupdate.rst > @@ -562,8 +562,8 @@ Command line parameters > | | | See below the internal command line | > | | | arguments for suricatta. | > +-------------+----------+--------------------------------------------+ > -| -H | string | Available on CONFIG_HW_COMPATIBILITY set. | > -| <board:rev> | | Set board name and hardware revision. | > +| -H | string | Set board name and hardware revision. | > +| <board:rev> | | | > +-------------+----------+--------------------------------------------+ > | -c | - | Check ``*.swu`` file. It ensures that files| > | | | referenced in sw-description are present. | Best regards, Stefano
Hi Stefano, thanks for your quick reply and a happy new year btw ;) > The switch is just CONFIG_HW_COMPATIBILITY. If this is not set, any value in > /etc/hw_revision is not read. SWUpdate always attempts to read hw_revision (swupdate.c calls get_hw_revision even if CONFIG_HW_COMPATIBILITY is disabled), although as you correctly said, the actual check is *not* performed (which is the expected behavior). > Setting a board name into sw-description, but disabling > CONFIG_HW_COMPATIBILITY is a sort of hybrid that is unsupported. It may be unsupported, but it's possible and what I somehow ended up with (since I do a custom hw revision check in Lua). I'm setting the boardname via hw_revision file (with some arbitrary version, since it's not checked) and then use the boardname in sw-description. I wanted to get rid of the hw_revision file and set the boardname via CLI instead - and that's when I noticed it's not possible, unless I enable CONFIG_HW_COMPATIBILITY and also enforce the version check (which I might end up doing now). So instead of this patch, how about changing the behavior so that SWUpdate does not set a boardname if CONFIG_HW_COMPATIBILITY is unset? It's a breaking change though, but in the future it would prevent people ending up with the same unsupported configuration as I have now. Kind Regards, Michael
diff --git a/core/swupdate.c b/core/swupdate.c index 34c9e34..edf563c 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -85,9 +85,7 @@ static struct option long_options[] = { {"file", required_argument, NULL, 'f'}, {"get-root", no_argument, NULL, 'g'}, {"help", no_argument, NULL, 'h'}, -#ifdef CONFIG_HW_COMPATIBILITY {"hwrevision", required_argument, NULL, 'H'}, -#endif {"image", required_argument, NULL, 'i'}, #ifdef CONFIG_SIGNED_IMAGES {"key", required_argument, NULL, 'k'}, @@ -165,9 +163,7 @@ static void usage(char *programname) " -o, --output <filename> : saves the incoming stream\n" " -v, --verbose : be verbose, set maximum loglevel\n" " --version : print SWUpdate version and exit\n" -#ifdef CONFIG_HW_COMPATIBILITY " -H, --hwrevision <board>:<rev> : Set hardware revision\n" -#endif " -c, --check : check image and exit, use with -i <filename>\n" " -h, --help : print this help and exit\n" ); @@ -447,7 +443,7 @@ int main(int argc, char **argv) #endif memset(main_options, 0, sizeof(main_options)); memset(image_url, 0, sizeof(image_url)); - strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:N:R:MmB:"); + strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:N:R:MmB:H:"); #ifdef CONFIG_MTD strcat(main_options, "b:"); #endif @@ -460,9 +456,6 @@ int main(int argc, char **argv) #ifdef CONFIG_WEBSERVER strcat(main_options, "w:"); #endif -#ifdef CONFIG_HW_COMPATIBILITY - strcat(main_options, "H:"); -#endif #ifdef CONFIG_SIGNED_IMAGES strcat(main_options, "k:"); public_key_mandatory = 1; diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst index 802b16a..4fcca22 100644 --- a/doc/source/swupdate.rst +++ b/doc/source/swupdate.rst @@ -562,8 +562,8 @@ Command line parameters | | | See below the internal command line | | | | arguments for suricatta. | +-------------+----------+--------------------------------------------+ -| -H | string | Available on CONFIG_HW_COMPATIBILITY set. | -| <board:rev> | | Set board name and hardware revision. | +| -H | string | Set board name and hardware revision. | +| <board:rev> | | | +-------------+----------+--------------------------------------------+ | -c | - | Check ``*.swu`` file. It ensures that files| | | | referenced in sw-description are present. |
There are two methods for populating struct hw_type: 1) using get_hw_revision (in util.c) 2) using the CLI argument -H Method 1) will either parse CONFIG_HW_COMPATIBILITY_FILE or use /etc/hwrevision if it's not set. Method 2) is only available if CONFIG_HW_COMPATIBILITY is set. This is inconsistent though, because if SWUpdate is built without CONFIG_HW_COMPATIBILITY, then the hardware boardname can still be set using /etc/hwrevision but not via CLI. Setting the hw boardname is useful even without checking hw revisions (parse_hw_compatibility), because it is also used in find_node_and_path (parser.c) to traverse sw-description and select the correct software. Signed-off-by: Michael Adler <michael.adler@siemens.com> --- core/swupdate.c | 9 +-------- doc/source/swupdate.rst | 4 ++-- 2 files changed, 3 insertions(+), 10 deletions(-)