[1/1] no-reinstalling feature

From: James Hilliard <james.hilliard1@gmail.com>

Some projects require a way to prevent reinstallation of the current firmware
version to prevent repeated updates. For such cases, a new command line
parameter is introduced (-R <current version>) to inform SWUpdate about the
current version. SWUpdate performs a simple string comparision to determine
if the current version is the same as the installed version. We do this so
that no-reinstalling can be used with any versioning format.

We also ensure that the no-downgrading feature doesn't prevent installation
of the existing firmware, it's expected that the no-downgrading feature is
used to enforce a minimum firmware version as opposed to preventing
reinstallation of the existing firmware version, there may be cases where
the minimum version is not the same as the current version, for example a
device may have multiple firmware versions to choose from with the minimum
required version being lower than the current version. For this reason we
enforce the no-downgrading requirement separately from no-reinstalling.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 core/parser.c           | 17 ++++++++--
 core/swupdate.c         | 73 ++++++++++++++++++++++++-----------------
 doc/source/swupdate.rst |  7 +++-
 include/swupdate.h      |  2 ++
 4 files changed, 65 insertions(+), 34 deletions(-)

Hi James,

On 19/02/19 10:07, james.hilliard1@gmail.com wrote:
> From: James Hilliard <james.hilliard1@gmail.com>
> 
> Some projects require a way to prevent reinstallation of the current firmware
> version to prevent repeated updates. For such cases, a new command line
> parameter is introduced (-R <current version>) to inform SWUpdate about the
> current version. SWUpdate performs a simple string comparision to determine
> if the current version is the same as the installed version. We do this so
> that no-reinstalling can be used with any versioning format.

I would say that the "reinstallation" of the current software reported
by Matthias depends on how it connects and starts SWUPdate to the rest
of the system. There is tons of way to avoid such as loop, and they
depend also from the selected interface for the update. In case of
Hawkbit, it is handled at server level.

I will focus the patch about version comparisons, let's say SWUpdate
gets the version from the running system and from the SWU (set via
version=..) and must decide in the most flexible way if the SWU is
accepted or not. "no-reinstalling" appears to me a fix for a special
case without having a general approach.

> 
> We also ensure that the no-downgrading feature doesn't prevent installation
> of the existing firmware, it's expected that the no-downgrading feature is
> used to enforce a minimum firmware version as opposed to preventing
> reinstallation of the existing firmware version, there may be cases where
> the minimum version is not the same as the current version,

But then it is not a "no-downgrading" feature, the name becomes misleading.

The major thing is that the no-downgrading feature was thought by me as
a global option, that means the version identifies the whole new release
delivered as single SWU. You use case does not match very well, because
you have multiple SWUs and each of them could have a version number. So
your use case is better approached if each SWU can be identified in some
way and it has an associate version (last part is easy, this is still
"version="), and SWUpdate when runs check for the SWU type (OS,
application, config, whatever...) and has a table with the installed
version number. For each of these version, a comparison check (> or >=)
should be done.

So I prefer to find a general solution, because this one does not scale
well: let's say we have OS that cannot be downgraded (even same version
is forbidden), but at the same time any version of application can be
installed, and so on.

> for example a
> device may have multiple firmware versions to choose from with the minimum
> required version being lower than the current version. For this reason we
> enforce the no-downgrading requirement separately from no-reinstalling.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  core/parser.c           | 17 ++++++++--
>  core/swupdate.c         | 73 ++++++++++++++++++++++++-----------------
>  doc/source/swupdate.rst |  7 +++-
>  include/swupdate.h      |  2 ++
>  4 files changed, 65 insertions(+), 34 deletions(-)
> 
> diff --git a/core/parser.c b/core/parser.c
> index 65dc8d1..313201f 100644
> --- a/core/parser.c
> +++ b/core/parser.c
> @@ -257,11 +257,24 @@ int parse(struct swupdate_cfg *sw, const char *descfile)
>  	 * newer version
>  	 */
>  	if (sw->globals.no_downgrading) {
> -		__u64 currentversion = version_to_number(sw->globals.current_version);
> +		__u64 minimum_version = version_to_number(sw->globals.minimum_version);

sw->globals.current_version has no other usage, so after dropping this
line it is dead code. Instead, you add a new variable to the structure
(minimum_version) with quite the same meaning.

I will also prefer to split between meaning of the data (=version
running on system) and rule to be applied (the next lines).

>  		__u64 newversion = version_to_number(sw->version);
>  
> -		if (newversion <= currentversion) {
> +		if (newversion < minimum_version) {

This is the rule - I do not exactly how, but it like to let it as much
configurable.

One other use case can simply be that a version can be installed if a
different from the running, but versions are strings instead of a
sequence of numbers (many projects like to have strings).

>  			ERROR("No downgrading allowed: new version %s <= installed %s",
> +				sw->version, sw->globals.minimum_version);
> +			return -EPERM;
> +		}
> +	}
> +
> +	/*
> +	 * If reinstalling is not allowed, compare
> +	 * version strings
> +	 */
> +	if (sw->globals.no_reinstalling) {
> +
> +		if (strcmp(sw->version, sw->globals.current_version) == 0) {
> +			ERROR("No reinstalling allowed: new version %s == installed %s",
>  				sw->version, sw->globals.current_version);
>  			return -EPERM;
>  		}
> diff --git a/core/swupdate.c b/core/swupdate.c
> index 4f3d9d6..b932eaa 100644
> --- a/core/swupdate.c
> +++ b/core/swupdate.c
> @@ -79,6 +79,7 @@ static struct option long_options[] = {
>  	{"output", required_argument, NULL, 'o'},
>  	{"dry-run", no_argument, NULL, 'n'},
>  	{"no-downgrading", required_argument, NULL, 'N'},
> +	{"no-reinstalling", required_argument, NULL, 'R'},
>  #ifdef CONFIG_SIGNED_IMAGES
>  	{"key", required_argument, NULL, 'k'},
>  	{"ca-path", required_argument, NULL, 'k'},
> @@ -117,51 +118,52 @@ static void usage(char *programname)
>  	fprintf(stdout, "Usage %s [OPTION]\n",
>  			programname);
>  	fprintf(stdout,
> -		" -f, --file <filename>          : configuration file to use\n"
> +		" -f, --file <filename>           : configuration file to use\n"

There should be a reason, but I can't get it. If there is some
formatting reasons, they should be addressed in a seaparate patch so we
know that just formatting was changed. In this patch, I have just
expected changes for -R and -N, but not for many other options.

>  #ifdef CONFIG_UBIATTACH
> -		" -b, --blacklist <list of mtd>  : MTDs that must not be scanned for UBI\n"
> -#endif
> -		" -p, --postupdate               : execute post-update command\n"
> -		" -e, --select <software>,<mode> : Select software images set and source\n"
> -		"                                  Ex.: stable,main\n"
> -		" -i, --image <filename>         : Software to be installed\n"
> -		" -l, --loglevel <level>         : logging level\n"
> -		" -L, --syslog                   : enable syslog logger\n"
> +		" -b, --blacklist <list of mtd>   : MTDs that must not be scanned for UBI\n"
> +#endif
> +		" -p, --postupdate                : execute post-update command\n"
> +		" -e, --select <software>,<mode>  : Select software images set and source\n"
> +		"                                   Ex.: stable,main\n"
> +		" -i, --image <filename>          : Software to be installed\n"
> +		" -l, --loglevel <level>          : logging level\n"
> +		" -L, --syslog                    : enable syslog logger\n"

For example, I cannot understand which are the changes here.

>  #ifdef CONFIG_SIGNED_IMAGES
> -		" -k, --key <public key file>    : file with public key to verify images\n"
> -		"     --cert-purpose <purpose>   : set expected certificate purpose\n"
> -		"                                  [emailProtection|codeSigning] (default: emailProtection)\n"
> -		"     --forced-signer-name <cn>  : set expected common name of signer certificate\n"
> -		"     --ca-path                  : path to the Certificate Authority (PEM)\n"
> +		" -k, --key <public key file>     : file with public key to verify images\n"
> +		"     --cert-purpose <purpose>    : set expected certificate purpose\n"
> +		"                                   [emailProtection|codeSigning] (default: emailProtection)\n"
> +		"     --forced-signer-name <cn>   : set expected common name of signer certificate\n"
> +		"     --ca-path                   : path to the Certificate Authority (PEM)\n"
>  #endif
>  #ifdef CONFIG_ENCRYPTED_IMAGES
> -		" -K, --key-aes <key file>       : the file contains the symmetric key to be used\n"
> -		"                                  to decrypt images\n"
> -#endif
> -		" -n, --dry-run                  : run SWUpdate without installing the software\n"
> -		" -N, --no-downgrading <version> : not install a release older as <version>\n"
> -		" -o, --output <output file>     : saves the incoming stream\n"
> -		" -v, --verbose                  : be verbose, set maximum loglevel\n"
> -		"     --version                  : print SWUpdate version and exit\n"
> +		" -K, --key-aes <key file>        : the file contains the symmetric key to be used\n"
> +		"                                   to decrypt images\n"
> +#endif
> +		" -n, --dry-run                   : run SWUpdate without installing the software\n"
> +		" -N, --no-downgrading <version>  : not install a release older as <version>\n"
> +		" -R, --no-reinstalling <version> : not install a release same as <version>\n"
> +		" -o, --output <output file>      : 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"
> +		" -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"
> +		" -c, --check                     : check image and exit, use with -i <filename>\n"
> +		" -h, --help                      : print this help and exit\n"
>  		);
>  #ifdef CONFIG_DOWNLOAD
>  	fprintf(stdout,
> -		" -d, --download [OPTIONS]       : Parameters to be passed to the downloader\n");
> +		" -d, --download [OPTIONS]        : Parameters to be passed to the downloader\n");
>  	download_print_help();
>  #endif
>  #ifdef CONFIG_SURICATTA
>  	fprintf(stdout,
> -		" -u, --suricatta [OPTIONS]      : Parameters to be passed to suricatta\n");
> +		" -u, --suricatta [OPTIONS]       : Parameters to be passed to suricatta\n");
>  	suricatta_print_help();
>  #endif
>  #ifdef CONFIG_WEBSERVER
>  	fprintf(stdout,
> -		" -w, --webserver [OPTIONS]      : Parameters to be passed to webserver\n");
> +		" -w, --webserver [OPTIONS]       : Parameters to be passed to webserver\n");
>  	mongoose_print_help();
>  #endif
>  }
> @@ -503,9 +505,13 @@ static int read_globals_settings(void *elem, void *data)
>  	get_field(LIBCFG_PARSER, elem, "loglevel", &sw->globals.loglevel);
>  	get_field(LIBCFG_PARSER, elem, "syslog", &sw->globals.syslog_enabled);
>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
> -				"no-downgrading", sw->globals.current_version);
> -	if (strlen(sw->globals.current_version))
> +				"no-downgrading", sw->globals.minimum_version);
> +	if (strlen(sw->globals.minimum_version))
>  		sw->globals.no_downgrading = 1;
> +	GET_FIELD_STRING(LIBCFG_PARSER, elem,
> +				"no-reinstalling", sw->globals.current_version);
> +	if (strlen(sw->globals.current_version))
> +		sw->globals.no_reinstalling = 1;
>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
>  				"cert-purpose", tmp);
>  	if (tmp[0] != '\0')
> @@ -598,7 +604,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:l:Lcf:p:o:N:");
> +	strcpy(main_options, "vhni:e:l:Lcf:p:o:N:R:");
>  #ifdef CONFIG_MTD
>  	strcat(main_options, "b:");
>  #endif
> @@ -741,6 +747,11 @@ int main(int argc, char **argv)
>  #endif
>  		case 'N':
>  			swcfg.globals.no_downgrading = 1;
> +			strncpy(swcfg.globals.minimum_version, optarg,
> +				sizeof(swcfg.globals.minimum_version));
> +			break;
> +		case 'R':
> +			swcfg.globals.no_reinstalling = 1;
>  			strncpy(swcfg.globals.current_version, optarg,
>  				sizeof(swcfg.globals.current_version));
>  			break;
> diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
> index 9e36924..8d664b7 100644
> --- a/doc/source/swupdate.rst
> +++ b/doc/source/swupdate.rst
> @@ -501,7 +501,7 @@ Command line parameters
>  +-------------+----------+--------------------------------------------+
>  | -n          |    -     | run SWUpdate in dry-run mode.              |
>  +-------------+----------+--------------------------------------------+
> -| -N          | string   | passed the current installed version of    |
> +| -N          | string   | passed the minimum required version of     |
>  |             |          | software. This will be checked with the    |
>  |             |          | version of new software and forbids        |
>  |             |          | downgrading.                               |
> @@ -509,6 +509,11 @@ Command line parameters
>  |             |          | major.minor.rev.build                      |
>  |             |          | each field is in the range 0..65535        |
>  +-------------+----------+--------------------------------------------+
> +| -R          | string   | passed the current installed version of    |
> +|             |          | software. This will be checked with the    |
> +|             |          | version of new software and forbids        |
> +|             |          | reinstalling.                              |
> ++-------------+----------+--------------------------------------------+
>  | -o <file>   | string   | saves the stream (SWU) on a file           |
>  +-------------+----------+--------------------------------------------+
>  | -v          |    -     | activate verbose output                    |
> diff --git a/include/swupdate.h b/include/swupdate.h
> index b54b904..69f2a7f 100644
> --- a/include/swupdate.h
> +++ b/include/swupdate.h
> @@ -111,9 +111,11 @@ struct swupdate_global_cfg {
>  	int syslog_enabled;
>  	int dry_run;
>  	int no_downgrading;
> +	int no_reinstalling;
>  	char publickeyfname[SWUPDATE_GENERAL_STRING_SIZE];
>  	char aeskeyfname[SWUPDATE_GENERAL_STRING_SIZE];
>  	char postupdatecmd[SWUPDATE_GENERAL_STRING_SIZE];
> +	char minimum_version[SWUPDATE_GENERAL_STRING_SIZE];
>  	char current_version[SWUPDATE_GENERAL_STRING_SIZE];
>  	int cert_purpose;
>  	char forced_signer_name[SWUPDATE_GENERAL_STRING_SIZE];
> 

Best regards,
Stefano Babic

On Tue, Feb 19, 2019 at 7:42 AM Stefano Babic <sbabic@denx.de> wrote:
>
> Hi James,
>
> On 19/02/19 10:07, james.hilliard1@gmail.com wrote:
> > From: James Hilliard <james.hilliard1@gmail.com>
> >
> > Some projects require a way to prevent reinstallation of the current firmware
> > version to prevent repeated updates. For such cases, a new command line
> > parameter is introduced (-R <current version>) to inform SWUpdate about the
> > current version. SWUpdate performs a simple string comparision to determine
> > if the current version is the same as the installed version. We do this so
> > that no-reinstalling can be used with any versioning format.
>
> I would say that the "reinstallation" of the current software reported
> by Matthias depends on how it connects and starts SWUPdate to the rest
> of the system. There is tons of way to avoid such as loop, and they
> depend also from the selected interface for the update. In case of
> Hawkbit, it is handled at server level.
>
> I will focus the patch about version comparisons, let's say SWUpdate
> gets the version from the running system and from the SWU (set via
> version=..) and must decide in the most flexible way if the SWU is
> accepted or not. "no-reinstalling" appears to me a fix for a special
> case without having a general approach.
>
> >
> > We also ensure that the no-downgrading feature doesn't prevent installation
> > of the existing firmware, it's expected that the no-downgrading feature is
> > used to enforce a minimum firmware version as opposed to preventing
> > reinstallation of the existing firmware version, there may be cases where
> > the minimum version is not the same as the current version,
>
> But then it is not a "no-downgrading" feature, the name becomes misleading.
Downgrading IMO implies installing an older version, from the option name I
would not expect no-downgrading to prevent reinstalling the current version.
>
> The major thing is that the no-downgrading feature was thought by me as
> a global option, that means the version identifies the whole new release
> delivered as single SWU. You use case does not match very well, because
> you have multiple SWUs and each of them could have a version number. So
> your use case is better approached if each SWU can be identified in some
> way and it has an associate version (last part is easy, this is still
> "version="), and SWUpdate when runs check for the SWU type (OS,
> application, config, whatever...) and has a table with the installed
> version number. For each of these version, a comparison check (> or >=)
> should be done.
In this instance I'm only really referring to the global option, for
example I may
produce some test builds or region specific builds where rollback is
safe as long
as the version is greater than a particular version.
>
> So I prefer to find a general solution, because this one does not scale
> well: let's say we have OS that cannot be downgraded (even same version
> is forbidden), but at the same time any version of application can be
> installed, and so on.
This would work for some of those use cases still as you could set the swu with
the application to a version higher than any OS versions.
>
> > for example a
> > device may have multiple firmware versions to choose from with the minimum
> > required version being lower than the current version. For this reason we
> > enforce the no-downgrading requirement separately from no-reinstalling.
> >
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >  core/parser.c           | 17 ++++++++--
> >  core/swupdate.c         | 73 ++++++++++++++++++++++++-----------------
> >  doc/source/swupdate.rst |  7 +++-
> >  include/swupdate.h      |  2 ++
> >  4 files changed, 65 insertions(+), 34 deletions(-)
> >
> > diff --git a/core/parser.c b/core/parser.c
> > index 65dc8d1..313201f 100644
> > --- a/core/parser.c
> > +++ b/core/parser.c
> > @@ -257,11 +257,24 @@ int parse(struct swupdate_cfg *sw, const char *descfile)
> >        * newer version
> >        */
> >       if (sw->globals.no_downgrading) {
> > -             __u64 currentversion = version_to_number(sw->globals.current_version);
> > +             __u64 minimum_version = version_to_number(sw->globals.minimum_version);
>
> sw->globals.current_version has no other usage, so after dropping this
> line it is dead code. Instead, you add a new variable to the structure
> (minimum_version) with quite the same meaning.
>
> I will also prefer to split between meaning of the data (=version
> running on system) and rule to be applied (the next lines).
>
> >               __u64 newversion = version_to_number(sw->version);
> >
> > -             if (newversion <= currentversion) {
> > +             if (newversion < minimum_version) {
>
> This is the rule - I do not exactly how, but it like to let it as much
> configurable.
Well it's configurable in the sense that you can set the no-downgrading
version to the same as no-reinstalling to get the <= behavior.
>
> One other use case can simply be that a version can be installed if a
> different from the running, but versions are strings instead of a
> sequence of numbers (many projects like to have strings).
It was my intention to cover that use case with no-reinstalling.
>
> >                       ERROR("No downgrading allowed: new version %s <= installed %s",
> > +                             sw->version, sw->globals.minimum_version);
> > +                     return -EPERM;
> > +             }
> > +     }
> > +
> > +     /*
> > +      * If reinstalling is not allowed, compare
> > +      * version strings
> > +      */
> > +     if (sw->globals.no_reinstalling) {
> > +
> > +             if (strcmp(sw->version, sw->globals.current_version) == 0) {
> > +                     ERROR("No reinstalling allowed: new version %s == installed %s",
> >                               sw->version, sw->globals.current_version);
> >                       return -EPERM;
> >               }
> > diff --git a/core/swupdate.c b/core/swupdate.c
> > index 4f3d9d6..b932eaa 100644
> > --- a/core/swupdate.c
> > +++ b/core/swupdate.c
> > @@ -79,6 +79,7 @@ static struct option long_options[] = {
> >       {"output", required_argument, NULL, 'o'},
> >       {"dry-run", no_argument, NULL, 'n'},
> >       {"no-downgrading", required_argument, NULL, 'N'},
> > +     {"no-reinstalling", required_argument, NULL, 'R'},
> >  #ifdef CONFIG_SIGNED_IMAGES
> >       {"key", required_argument, NULL, 'k'},
> >       {"ca-path", required_argument, NULL, 'k'},
> > @@ -117,51 +118,52 @@ static void usage(char *programname)
> >       fprintf(stdout, "Usage %s [OPTION]\n",
> >                       programname);
> >       fprintf(stdout,
> > -             " -f, --file <filename>          : configuration file to use\n"
> > +             " -f, --file <filename>           : configuration file to use\n"
>
> There should be a reason, but I can't get it. If there is some
> formatting reasons, they should be addressed in a seaparate patch so we
> know that just formatting was changed. In this patch, I have just
> expected changes for -R and -N, but not for many other options.
Yeah, this was due to an alignment issue, I had to add an extra space to get
no-reinstalling properly aligned with the other options.
>
> >  #ifdef CONFIG_UBIATTACH
> > -             " -b, --blacklist <list of mtd>  : MTDs that must not be scanned for UBI\n"
> > -#endif
> > -             " -p, --postupdate               : execute post-update command\n"
> > -             " -e, --select <software>,<mode> : Select software images set and source\n"
> > -             "                                  Ex.: stable,main\n"
> > -             " -i, --image <filename>         : Software to be installed\n"
> > -             " -l, --loglevel <level>         : logging level\n"
> > -             " -L, --syslog                   : enable syslog logger\n"
> > +             " -b, --blacklist <list of mtd>   : MTDs that must not be scanned for UBI\n"
> > +#endif
> > +             " -p, --postupdate                : execute post-update command\n"
> > +             " -e, --select <software>,<mode>  : Select software images set and source\n"
> > +             "                                   Ex.: stable,main\n"
> > +             " -i, --image <filename>          : Software to be installed\n"
> > +             " -l, --loglevel <level>          : logging level\n"
> > +             " -L, --syslog                    : enable syslog logger\n"
>
> For example, I cannot understand which are the changes here.
>
> >  #ifdef CONFIG_SIGNED_IMAGES
> > -             " -k, --key <public key file>    : file with public key to verify images\n"
> > -             "     --cert-purpose <purpose>   : set expected certificate purpose\n"
> > -             "                                  [emailProtection|codeSigning] (default: emailProtection)\n"
> > -             "     --forced-signer-name <cn>  : set expected common name of signer certificate\n"
> > -             "     --ca-path                  : path to the Certificate Authority (PEM)\n"
> > +             " -k, --key <public key file>     : file with public key to verify images\n"
> > +             "     --cert-purpose <purpose>    : set expected certificate purpose\n"
> > +             "                                   [emailProtection|codeSigning] (default: emailProtection)\n"
> > +             "     --forced-signer-name <cn>   : set expected common name of signer certificate\n"
> > +             "     --ca-path                   : path to the Certificate Authority (PEM)\n"
> >  #endif
> >  #ifdef CONFIG_ENCRYPTED_IMAGES
> > -             " -K, --key-aes <key file>       : the file contains the symmetric key to be used\n"
> > -             "                                  to decrypt images\n"
> > -#endif
> > -             " -n, --dry-run                  : run SWUpdate without installing the software\n"
> > -             " -N, --no-downgrading <version> : not install a release older as <version>\n"
> > -             " -o, --output <output file>     : saves the incoming stream\n"
> > -             " -v, --verbose                  : be verbose, set maximum loglevel\n"
> > -             "     --version                  : print SWUpdate version and exit\n"
> > +             " -K, --key-aes <key file>        : the file contains the symmetric key to be used\n"
> > +             "                                   to decrypt images\n"
> > +#endif
> > +             " -n, --dry-run                   : run SWUpdate without installing the software\n"
> > +             " -N, --no-downgrading <version>  : not install a release older as <version>\n"
> > +             " -R, --no-reinstalling <version> : not install a release same as <version>\n"
> > +             " -o, --output <output file>      : 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"
> > +             " -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"
> > +             " -c, --check                     : check image and exit, use with -i <filename>\n"
> > +             " -h, --help                      : print this help and exit\n"
> >               );
> >  #ifdef CONFIG_DOWNLOAD
> >       fprintf(stdout,
> > -             " -d, --download [OPTIONS]       : Parameters to be passed to the downloader\n");
> > +             " -d, --download [OPTIONS]        : Parameters to be passed to the downloader\n");
> >       download_print_help();
> >  #endif
> >  #ifdef CONFIG_SURICATTA
> >       fprintf(stdout,
> > -             " -u, --suricatta [OPTIONS]      : Parameters to be passed to suricatta\n");
> > +             " -u, --suricatta [OPTIONS]       : Parameters to be passed to suricatta\n");
> >       suricatta_print_help();
> >  #endif
> >  #ifdef CONFIG_WEBSERVER
> >       fprintf(stdout,
> > -             " -w, --webserver [OPTIONS]      : Parameters to be passed to webserver\n");
> > +             " -w, --webserver [OPTIONS]       : Parameters to be passed to webserver\n");
> >       mongoose_print_help();
> >  #endif
> >  }
> > @@ -503,9 +505,13 @@ static int read_globals_settings(void *elem, void *data)
> >       get_field(LIBCFG_PARSER, elem, "loglevel", &sw->globals.loglevel);
> >       get_field(LIBCFG_PARSER, elem, "syslog", &sw->globals.syslog_enabled);
> >       GET_FIELD_STRING(LIBCFG_PARSER, elem,
> > -                             "no-downgrading", sw->globals.current_version);
> > -     if (strlen(sw->globals.current_version))
> > +                             "no-downgrading", sw->globals.minimum_version);
> > +     if (strlen(sw->globals.minimum_version))
> >               sw->globals.no_downgrading = 1;
> > +     GET_FIELD_STRING(LIBCFG_PARSER, elem,
> > +                             "no-reinstalling", sw->globals.current_version);
> > +     if (strlen(sw->globals.current_version))
> > +             sw->globals.no_reinstalling = 1;
> >       GET_FIELD_STRING(LIBCFG_PARSER, elem,
> >                               "cert-purpose", tmp);
> >       if (tmp[0] != '\0')
> > @@ -598,7 +604,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:l:Lcf:p:o:N:");
> > +     strcpy(main_options, "vhni:e:l:Lcf:p:o:N:R:");
> >  #ifdef CONFIG_MTD
> >       strcat(main_options, "b:");
> >  #endif
> > @@ -741,6 +747,11 @@ int main(int argc, char **argv)
> >  #endif
> >               case 'N':
> >                       swcfg.globals.no_downgrading = 1;
> > +                     strncpy(swcfg.globals.minimum_version, optarg,
> > +                             sizeof(swcfg.globals.minimum_version));
> > +                     break;
> > +             case 'R':
> > +                     swcfg.globals.no_reinstalling = 1;
> >                       strncpy(swcfg.globals.current_version, optarg,
> >                               sizeof(swcfg.globals.current_version));
> >                       break;
> > diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
> > index 9e36924..8d664b7 100644
> > --- a/doc/source/swupdate.rst
> > +++ b/doc/source/swupdate.rst
> > @@ -501,7 +501,7 @@ Command line parameters
> >  +-------------+----------+--------------------------------------------+
> >  | -n          |    -     | run SWUpdate in dry-run mode.              |
> >  +-------------+----------+--------------------------------------------+
> > -| -N          | string   | passed the current installed version of    |
> > +| -N          | string   | passed the minimum required version of     |
> >  |             |          | software. This will be checked with the    |
> >  |             |          | version of new software and forbids        |
> >  |             |          | downgrading.                               |
> > @@ -509,6 +509,11 @@ Command line parameters
> >  |             |          | major.minor.rev.build                      |
> >  |             |          | each field is in the range 0..65535        |
> >  +-------------+----------+--------------------------------------------+
> > +| -R          | string   | passed the current installed version of    |
> > +|             |          | software. This will be checked with the    |
> > +|             |          | version of new software and forbids        |
> > +|             |          | reinstalling.                              |
> > ++-------------+----------+--------------------------------------------+
> >  | -o <file>   | string   | saves the stream (SWU) on a file           |
> >  +-------------+----------+--------------------------------------------+
> >  | -v          |    -     | activate verbose output                    |
> > diff --git a/include/swupdate.h b/include/swupdate.h
> > index b54b904..69f2a7f 100644
> > --- a/include/swupdate.h
> > +++ b/include/swupdate.h
> > @@ -111,9 +111,11 @@ struct swupdate_global_cfg {
> >       int syslog_enabled;
> >       int dry_run;
> >       int no_downgrading;
> > +     int no_reinstalling;
> >       char publickeyfname[SWUPDATE_GENERAL_STRING_SIZE];
> >       char aeskeyfname[SWUPDATE_GENERAL_STRING_SIZE];
> >       char postupdatecmd[SWUPDATE_GENERAL_STRING_SIZE];
> > +     char minimum_version[SWUPDATE_GENERAL_STRING_SIZE];
> >       char current_version[SWUPDATE_GENERAL_STRING_SIZE];
> >       int cert_purpose;
> >       char forced_signer_name[SWUPDATE_GENERAL_STRING_SIZE];
> >
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================

Hi James,

On 20/02/19 11:51, James Hilliard wrote:
> On Tue, Feb 19, 2019 at 7:42 AM Stefano Babic <sbabic@denx.de> wrote:
>>
>> Hi James,
>>
>> On 19/02/19 10:07, james.hilliard1@gmail.com wrote:
>>> From: James Hilliard <james.hilliard1@gmail.com>
>>>
>>> Some projects require a way to prevent reinstallation of the current firmware
>>> version to prevent repeated updates. For such cases, a new command line
>>> parameter is introduced (-R <current version>) to inform SWUpdate about the
>>> current version. SWUpdate performs a simple string comparision to determine
>>> if the current version is the same as the installed version. We do this so
>>> that no-reinstalling can be used with any versioning format.
>>
>> I would say that the "reinstallation" of the current software reported
>> by Matthias depends on how it connects and starts SWUPdate to the rest
>> of the system. There is tons of way to avoid such as loop, and they
>> depend also from the selected interface for the update. In case of
>> Hawkbit, it is handled at server level.
>>
>> I will focus the patch about version comparisons, let's say SWUpdate
>> gets the version from the running system and from the SWU (set via
>> version=..) and must decide in the most flexible way if the SWU is
>> accepted or not. "no-reinstalling" appears to me a fix for a special
>> case without having a general approach.
>>
>>>
>>> We also ensure that the no-downgrading feature doesn't prevent installation
>>> of the existing firmware, it's expected that the no-downgrading feature is
>>> used to enforce a minimum firmware version as opposed to preventing
>>> reinstallation of the existing firmware version, there may be cases where
>>> the minimum version is not the same as the current version,
>>
>> But then it is not a "no-downgrading" feature, the name becomes misleading.
> Downgrading IMO implies installing an older version, from the option name I
> would not expect no-downgrading to prevent reinstalling the current version.
>>
>> The major thing is that the no-downgrading feature was thought by me as
>> a global option, that means the version identifies the whole new release
>> delivered as single SWU. You use case does not match very well, because
>> you have multiple SWUs and each of them could have a version number. So
>> your use case is better approached if each SWU can be identified in some
>> way and it has an associate version (last part is easy, this is still
>> "version="), and SWUpdate when runs check for the SWU type (OS,
>> application, config, whatever...) and has a table with the installed
>> version number. For each of these version, a comparison check (> or >=)
>> should be done.
> In this instance I'm only really referring to the global option, for
> example I may
> produce some test builds or region specific builds where rollback is
> safe as long
> as the version is greater than a particular version.

ok, understood.

>>
>> So I prefer to find a general solution, because this one does not scale
>> well: let's say we have OS that cannot be downgraded (even same version
>> is forbidden), but at the same time any version of application can be
>> installed, and so on.
> This would work for some of those use cases still as you could set the swu with
> the application to a version higher than any OS versions.

Right - I am not yet sure how a generic check can be implemented. One
idea I have is that sw-description *must* have (if SWUpdate is
configured for check) a sort of "version-check" function that must be
called. This could be general, because SWUpdate has not implemented the
rule. The rule itself is defined by the user itself when he prepares the
release, and it could be a simple check or a more complex test - the
user can define a whole script for that. Anyway, SWUpdate should at
least make availabel to Lua the fuctions to convert versions and it must
be ensured that a SWU without that check is rejected.

But let's start with step: it is ok for me to get your patch now to fix
a current issue, and move to a more generic approach later.

>>
>>> for example a
>>> device may have multiple firmware versions to choose from with the minimum
>>> required version being lower than the current version. For this reason we
>>> enforce the no-downgrading requirement separately from no-reinstalling.
>>>
>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>>> ---
>>>  core/parser.c           | 17 ++++++++--
>>>  core/swupdate.c         | 73 ++++++++++++++++++++++++-----------------
>>>  doc/source/swupdate.rst |  7 +++-
>>>  include/swupdate.h      |  2 ++
>>>  4 files changed, 65 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/core/parser.c b/core/parser.c
>>> index 65dc8d1..313201f 100644
>>> --- a/core/parser.c
>>> +++ b/core/parser.c
>>> @@ -257,11 +257,24 @@ int parse(struct swupdate_cfg *sw, const char *descfile)
>>>        * newer version
>>>        */
>>>       if (sw->globals.no_downgrading) {
>>> -             __u64 currentversion = version_to_number(sw->globals.current_version);
>>> +             __u64 minimum_version = version_to_number(sw->globals.minimum_version);
>>
>> sw->globals.current_version has no other usage, so after dropping this
>> line it is dead code. Instead, you add a new variable to the structure
>> (minimum_version) with quite the same meaning.
>>
>> I will also prefer to split between meaning of the data (=version
>> running on system) and rule to be applied (the next lines).
>>
>>>               __u64 newversion = version_to_number(sw->version);
>>>
>>> -             if (newversion <= currentversion) {
>>> +             if (newversion < minimum_version) {
>>
>> This is the rule - I do not exactly how, but it like to let it as much
>> configurable.
> Well it's configurable in the sense that you can set the no-downgrading
> version to the same as no-reinstalling to get the <= behavior.

Yes - a little configurable. Just a bit that correspond to have "<" or
"<=". Anyway, I am fine for now - just please reformat the patch to
avoid not reviewable changes due to alignment. or move these changes to
a second patch.

>>
>> One other use case can simply be that a version can be installed if a
>> different from the running, but versions are strings instead of a
>> sequence of numbers (many projects like to have strings).
> It was my intention to cover that use case with no-reinstalling.

Quite - this covers just a different version, while there are cases
where the Version string means something more (with device name, line of
product, vendor, and so on) and the string must be parsed in some way to
reduce it to a number. For such as generic cases, I tend to propose an
approach with Lua as ttrying to force it into the C code.

>>
>>>                       ERROR("No downgrading allowed: new version %s <= installed %s",
>>> +                             sw->version, sw->globals.minimum_version);
>>> +                     return -EPERM;
>>> +             }
>>> +     }
>>> +
>>> +     /*
>>> +      * If reinstalling is not allowed, compare
>>> +      * version strings
>>> +      */
>>> +     if (sw->globals.no_reinstalling) {
>>> +
>>> +             if (strcmp(sw->version, sw->globals.current_version) == 0) {
>>> +                     ERROR("No reinstalling allowed: new version %s == installed %s",
>>>                               sw->version, sw->globals.current_version);
>>>                       return -EPERM;
>>>               }
>>> diff --git a/core/swupdate.c b/core/swupdate.c
>>> index 4f3d9d6..b932eaa 100644
>>> --- a/core/swupdate.c
>>> +++ b/core/swupdate.c
>>> @@ -79,6 +79,7 @@ static struct option long_options[] = {
>>>       {"output", required_argument, NULL, 'o'},
>>>       {"dry-run", no_argument, NULL, 'n'},
>>>       {"no-downgrading", required_argument, NULL, 'N'},
>>> +     {"no-reinstalling", required_argument, NULL, 'R'},
>>>  #ifdef CONFIG_SIGNED_IMAGES
>>>       {"key", required_argument, NULL, 'k'},
>>>       {"ca-path", required_argument, NULL, 'k'},
>>> @@ -117,51 +118,52 @@ static void usage(char *programname)
>>>       fprintf(stdout, "Usage %s [OPTION]\n",
>>>                       programname);
>>>       fprintf(stdout,
>>> -             " -f, --file <filename>          : configuration file to use\n"
>>> +             " -f, --file <filename>           : configuration file to use\n"
>>
>> There should be a reason, but I can't get it. If there is some
>> formatting reasons, they should be addressed in a seaparate patch so we
>> know that just formatting was changed. In this patch, I have just
>> expected changes for -R and -N, but not for many other options.
> Yeah, this was due to an alignment issue, I had to add an extra space to get
> no-reinstalling properly aligned with the other options.

Ok, two propoasal:

- you rename no-reinstalling with some shorter name (do not ask me,
english is not my mother language, you are in a better position..) or
you split this patch into two where the second one fixes just the
alignment (according to the principle : one commit, one issue).

>>
>>>  #ifdef CONFIG_UBIATTACH
>>> -             " -b, --blacklist <list of mtd>  : MTDs that must not be scanned for UBI\n"
>>> -#endif
>>> -             " -p, --postupdate               : execute post-update command\n"
>>> -             " -e, --select <software>,<mode> : Select software images set and source\n"
>>> -             "                                  Ex.: stable,main\n"
>>> -             " -i, --image <filename>         : Software to be installed\n"
>>> -             " -l, --loglevel <level>         : logging level\n"
>>> -             " -L, --syslog                   : enable syslog logger\n"
>>> +             " -b, --blacklist <list of mtd>   : MTDs that must not be scanned for UBI\n"
>>> +#endif
>>> +             " -p, --postupdate                : execute post-update command\n"
>>> +             " -e, --select <software>,<mode>  : Select software images set and source\n"
>>> +             "                                   Ex.: stable,main\n"
>>> +             " -i, --image <filename>          : Software to be installed\n"
>>> +             " -l, --loglevel <level>          : logging level\n"
>>> +             " -L, --syslog                    : enable syslog logger\n"
>>
>> For example, I cannot understand which are the changes here.
>>
>>>  #ifdef CONFIG_SIGNED_IMAGES
>>> -             " -k, --key <public key file>    : file with public key to verify images\n"
>>> -             "     --cert-purpose <purpose>   : set expected certificate purpose\n"
>>> -             "                                  [emailProtection|codeSigning] (default: emailProtection)\n"
>>> -             "     --forced-signer-name <cn>  : set expected common name of signer certificate\n"
>>> -             "     --ca-path                  : path to the Certificate Authority (PEM)\n"
>>> +             " -k, --key <public key file>     : file with public key to verify images\n"
>>> +             "     --cert-purpose <purpose>    : set expected certificate purpose\n"
>>> +             "                                   [emailProtection|codeSigning] (default: emailProtection)\n"
>>> +             "     --forced-signer-name <cn>   : set expected common name of signer certificate\n"
>>> +             "     --ca-path                   : path to the Certificate Authority (PEM)\n"
>>>  #endif
>>>  #ifdef CONFIG_ENCRYPTED_IMAGES
>>> -             " -K, --key-aes <key file>       : the file contains the symmetric key to be used\n"
>>> -             "                                  to decrypt images\n"
>>> -#endif
>>> -             " -n, --dry-run                  : run SWUpdate without installing the software\n"
>>> -             " -N, --no-downgrading <version> : not install a release older as <version>\n"
>>> -             " -o, --output <output file>     : saves the incoming stream\n"
>>> -             " -v, --verbose                  : be verbose, set maximum loglevel\n"
>>> -             "     --version                  : print SWUpdate version and exit\n"
>>> +             " -K, --key-aes <key file>        : the file contains the symmetric key to be used\n"
>>> +             "                                   to decrypt images\n"
>>> +#endif
>>> +             " -n, --dry-run                   : run SWUpdate without installing the software\n"
>>> +             " -N, --no-downgrading <version>  : not install a release older as <version>\n"
>>> +             " -R, --no-reinstalling <version> : not install a release same as <version>\n"
>>> +             " -o, --output <output file>      : 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"
>>> +             " -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"
>>> +             " -c, --check                     : check image and exit, use with -i <filename>\n"
>>> +             " -h, --help                      : print this help and exit\n"
>>>               );
>>>  #ifdef CONFIG_DOWNLOAD
>>>       fprintf(stdout,
>>> -             " -d, --download [OPTIONS]       : Parameters to be passed to the downloader\n");
>>> +             " -d, --download [OPTIONS]        : Parameters to be passed to the downloader\n");
>>>       download_print_help();
>>>  #endif
>>>  #ifdef CONFIG_SURICATTA
>>>       fprintf(stdout,
>>> -             " -u, --suricatta [OPTIONS]      : Parameters to be passed to suricatta\n");
>>> +             " -u, --suricatta [OPTIONS]       : Parameters to be passed to suricatta\n");
>>>       suricatta_print_help();
>>>  #endif
>>>  #ifdef CONFIG_WEBSERVER
>>>       fprintf(stdout,
>>> -             " -w, --webserver [OPTIONS]      : Parameters to be passed to webserver\n");
>>> +             " -w, --webserver [OPTIONS]       : Parameters to be passed to webserver\n");
>>>       mongoose_print_help();
>>>  #endif
>>>  }
>>> @@ -503,9 +505,13 @@ static int read_globals_settings(void *elem, void *data)
>>>       get_field(LIBCFG_PARSER, elem, "loglevel", &sw->globals.loglevel);
>>>       get_field(LIBCFG_PARSER, elem, "syslog", &sw->globals.syslog_enabled);
>>>       GET_FIELD_STRING(LIBCFG_PARSER, elem,
>>> -                             "no-downgrading", sw->globals.current_version);
>>> -     if (strlen(sw->globals.current_version))
>>> +                             "no-downgrading", sw->globals.minimum_version);
>>> +     if (strlen(sw->globals.minimum_version))
>>>               sw->globals.no_downgrading = 1;
>>> +     GET_FIELD_STRING(LIBCFG_PARSER, elem,
>>> +                             "no-reinstalling", sw->globals.current_version);
>>> +     if (strlen(sw->globals.current_version))
>>> +             sw->globals.no_reinstalling = 1;
>>>       GET_FIELD_STRING(LIBCFG_PARSER, elem,
>>>                               "cert-purpose", tmp);
>>>       if (tmp[0] != '\0')
>>> @@ -598,7 +604,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:l:Lcf:p:o:N:");
>>> +     strcpy(main_options, "vhni:e:l:Lcf:p:o:N:R:");
>>>  #ifdef CONFIG_MTD
>>>       strcat(main_options, "b:");
>>>  #endif
>>> @@ -741,6 +747,11 @@ int main(int argc, char **argv)
>>>  #endif
>>>               case 'N':
>>>                       swcfg.globals.no_downgrading = 1;
>>> +                     strncpy(swcfg.globals.minimum_version, optarg,
>>> +                             sizeof(swcfg.globals.minimum_version));
>>> +                     break;
>>> +             case 'R':
>>> +                     swcfg.globals.no_reinstalling = 1;
>>>                       strncpy(swcfg.globals.current_version, optarg,
>>>                               sizeof(swcfg.globals.current_version));
>>>                       break;
>>> diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
>>> index 9e36924..8d664b7 100644
>>> --- a/doc/source/swupdate.rst
>>> +++ b/doc/source/swupdate.rst
>>> @@ -501,7 +501,7 @@ Command line parameters
>>>  +-------------+----------+--------------------------------------------+
>>>  | -n          |    -     | run SWUpdate in dry-run mode.              |
>>>  +-------------+----------+--------------------------------------------+
>>> -| -N          | string   | passed the current installed version of    |
>>> +| -N          | string   | passed the minimum required version of     |
>>>  |             |          | software. This will be checked with the    |
>>>  |             |          | version of new software and forbids        |
>>>  |             |          | downgrading.                               |
>>> @@ -509,6 +509,11 @@ Command line parameters
>>>  |             |          | major.minor.rev.build                      |
>>>  |             |          | each field is in the range 0..65535        |
>>>  +-------------+----------+--------------------------------------------+
>>> +| -R          | string   | passed the current installed version of    |
>>> +|             |          | software. This will be checked with the    |
>>> +|             |          | version of new software and forbids        |
>>> +|             |          | reinstalling.                              |
>>> ++-------------+----------+--------------------------------------------+
>>>  | -o <file>   | string   | saves the stream (SWU) on a file           |
>>>  +-------------+----------+--------------------------------------------+
>>>  | -v          |    -     | activate verbose output                    |
>>> diff --git a/include/swupdate.h b/include/swupdate.h
>>> index b54b904..69f2a7f 100644
>>> --- a/include/swupdate.h
>>> +++ b/include/swupdate.h
>>> @@ -111,9 +111,11 @@ struct swupdate_global_cfg {
>>>       int syslog_enabled;
>>>       int dry_run;
>>>       int no_downgrading;
>>> +     int no_reinstalling;
>>>       char publickeyfname[SWUPDATE_GENERAL_STRING_SIZE];
>>>       char aeskeyfname[SWUPDATE_GENERAL_STRING_SIZE];
>>>       char postupdatecmd[SWUPDATE_GENERAL_STRING_SIZE];
>>> +     char minimum_version[SWUPDATE_GENERAL_STRING_SIZE];
>>>       char current_version[SWUPDATE_GENERAL_STRING_SIZE];
>>>       int cert_purpose;
>>>       char forced_signer_name[SWUPDATE_GENERAL_STRING_SIZE];
>>>
Best regards,
Stefano