[2/2] core: blacklisting should depend on CONFIG_UBIATTACH

The blacklisting options should depend on CONFIG_UBIATTACH instead of
CONFIG_MTD. Indeed, blacklisting is only relevant for attaching a UBI
device. Thus, convert the CONFIG_MTD ifdefs into CONFIG_UBIATTACH in
the relevant locations.

In addition, there were two places where the ifdef was missing:
- in the parsing of swupdate.cfg;
- in the definition of struct swupdate_global_cfg.
So add the ifdef there.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
If patch 1/2 is not applied, this patch is still relevant but then
with UBIVOL instead of UBIATTACH. I'll respin as needed.
---
 core/swupdate.c         | 10 ++++++----
 doc/source/swupdate.rst |  2 +-
 include/swupdate.h      |  2 ++
 3 files changed, 9 insertions(+), 5 deletions(-)

On 21/08/2018 16:56, Arnout Vandecappelle (Essensium/Mind) wrote:
> The blacklisting options should depend on CONFIG_UBIATTACH instead of
> CONFIG_MTD. Indeed, blacklisting is only relevant for attaching a UBI
> device. Thus, convert the CONFIG_MTD ifdefs into CONFIG_UBIATTACH in
> the relevant locations.
> 
> In addition, there were two places where the ifdef was missing:
> - in the parsing of swupdate.cfg;
> - in the definition of struct swupdate_global_cfg.

Yes - I guess is not the only case. I have defined the structure for
all cases and I avoided #ifdef in those structure because the file
becomes less readable. The parsing needs also to not be surrounded
because all what it does is to copy the data in the structure, but
nothing is evaluated. I prefer this as to fill the header with #ifdef
that make the it less readable. Surrounding #ifdef for each field in the
structure means that we even must surround publickeyfname in
swupdate_global_cfg if CONFIG_SIGNED is not set, or aeskeyfname if
encryption is disabled, and much more.

I prefer to have a clean structure as to spare some bytes using a lot of
CONFIG_

> So add the ifdef there.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
> If patch 1/2 is not applied, this patch is still relevant but then
> with UBIVOL instead of UBIATTACH. I'll respin as needed.
> ---
>  core/swupdate.c         | 10 ++++++----
>  doc/source/swupdate.rst |  2 +-
>  include/swupdate.h      |  2 ++
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/core/swupdate.c b/core/swupdate.c
> index bc46e7a..f66f59c 100644
> --- a/core/swupdate.c
> +++ b/core/swupdate.c
> @@ -85,7 +85,7 @@ static struct option long_options[] = {
>  #ifdef CONFIG_ENCRYPTED_IMAGES
>  	{"key-aes", required_argument, NULL, 'K'},
>  #endif
> -#ifdef CONFIG_MTD
> +#ifdef CONFIG_UBIATTACH
>  	{"blacklist", required_argument, NULL, 'b'},
>  #endif
>  	{"help", no_argument, NULL, 'h'},
> @@ -115,7 +115,7 @@ static void usage(char *programname)
>  			programname);
>  	fprintf(stdout,
>  		" -f, --file <filename>          : configuration file to use\n"
> -#ifdef CONFIG_MTD
> +#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"
> @@ -469,8 +469,10 @@ static int read_globals_settings(void *elem, void *data)
>  				"public-key-file", sw->globals.publickeyfname);
>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
>  				"aes-key-file", sw->globals.aeskeyfname);
> +#ifdef CONFIG_UBIATTACH
>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
>  				"mtd-blacklist", sw->globals.mtdblacklist);
> +#endif
>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
>  				"postupdatecmd", sw->globals.postupdatecmd);
>  	get_field(LIBCFG_PARSER, elem, "verbose", &sw->globals.verbose);
> @@ -667,7 +669,7 @@ int main(int argc, char **argv)
>  		case 'v':
>  			loglevel = TRACELEVEL;
>  			break;
> -#ifdef CONFIG_MTD
> +#ifdef CONFIG_UBIATTACH
>  		case 'b':
>  			mtd_set_ubiblacklist(optarg);
>  			break;
> @@ -863,7 +865,7 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -#ifdef CONFIG_MTD
> +#ifdef CONFIG_UBIATTACH
>  	if (strlen(swcfg.globals.mtdblacklist))
>  		mtd_set_ubiblacklist(swcfg.globals.mtdblacklist);
>  #endif
> diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
> index fa59383..75fc4fb 100644
> --- a/doc/source/swupdate.rst
> +++ b/doc/source/swupdate.rst
> @@ -472,7 +472,7 @@ Command line parameters
>  +=============+==========+============================================+
>  | -f <file>   | string   | SWUpdate config file to use                |
>  +-------------+----------+--------------------------------------------+
> -| -b <string> | string   | Active only if CONFIG_MTD is set           |
> +| -b <string> | string   | Active only if CONFIG_UBIATTACH is set     |
>  |             |          | It allows to blacklist MTDs when SWUpdate  |
>  |             |          | searches for UBI volumes.                  |
>  |             |          | Example: U-Boot and environment in MTD0-1: |
> diff --git a/include/swupdate.h b/include/swupdate.h
> index 741d24c..83e8d3a 100644
> --- a/include/swupdate.h
> +++ b/include/swupdate.h
> @@ -103,7 +103,9 @@ enum {
>  
>  struct swupdate_global_cfg {
>  	int verbose;
> +#ifdef CONFIG_UBIATTACH
>  	char mtdblacklist[SWUPDATE_GENERAL_STRING_SIZE];
> +#endif
>  	int loglevel;
>  	int syslog_enabled;
>  	int dry_run;
> 

Reviewed-by: Stefano Babic <sbbic@denx.de>

Best regards,
Stefano Babic

On 22/08/2018 17:44, Stefano Babic wrote:
> On 21/08/2018 16:56, Arnout Vandecappelle (Essensium/Mind) wrote:
>> The blacklisting options should depend on CONFIG_UBIATTACH instead of
>> CONFIG_MTD. Indeed, blacklisting is only relevant for attaching a UBI
>> device. Thus, convert the CONFIG_MTD ifdefs into CONFIG_UBIATTACH in
>> the relevant locations.
>>
>> In addition, there were two places where the ifdef was missing:
>> - in the parsing of swupdate.cfg;
>> - in the definition of struct swupdate_global_cfg.
> 
> Yes - I guess is not the only case. I have defined the structure for
> all cases and I avoided #ifdef in those structure because the file
> becomes less readable.

 For me, it was convenient to really remove it from the structure, because then
the compiler will tell me if there is some place that I forgot to update.

 But such a change doesn't need to be committed, of course.

> The parsing needs also to not be surrounded
> because all what it does is to copy the data in the structure, but
> nothing is evaluated. I prefer this as to fill the header with #ifdef
> that make the it less readable. Surrounding #ifdef for each field in the
> structure means that we even must surround publickeyfname in
> swupdate_global_cfg if CONFIG_SIGNED is not set, or aeskeyfname if
> encryption is disabled, and much more.
> 
> I prefer to have a clean structure as to spare some bytes using a lot of
> CONFIG_

 OK, I'll respin with those two changes removed (and with your Reviewed-by).

 Regards,
 Arnout

> 
>> So add the ifdef there.
>>
>> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
>> ---
>> If patch 1/2 is not applied, this patch is still relevant but then
>> with UBIVOL instead of UBIATTACH. I'll respin as needed.
>> ---
>>  core/swupdate.c         | 10 ++++++----
>>  doc/source/swupdate.rst |  2 +-
>>  include/swupdate.h      |  2 ++
>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/core/swupdate.c b/core/swupdate.c
>> index bc46e7a..f66f59c 100644
>> --- a/core/swupdate.c
>> +++ b/core/swupdate.c
>> @@ -85,7 +85,7 @@ static struct option long_options[] = {
>>  #ifdef CONFIG_ENCRYPTED_IMAGES
>>  	{"key-aes", required_argument, NULL, 'K'},
>>  #endif
>> -#ifdef CONFIG_MTD
>> +#ifdef CONFIG_UBIATTACH
>>  	{"blacklist", required_argument, NULL, 'b'},
>>  #endif
>>  	{"help", no_argument, NULL, 'h'},
>> @@ -115,7 +115,7 @@ static void usage(char *programname)
>>  			programname);
>>  	fprintf(stdout,
>>  		" -f, --file <filename>          : configuration file to use\n"
>> -#ifdef CONFIG_MTD
>> +#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"
>> @@ -469,8 +469,10 @@ static int read_globals_settings(void *elem, void *data)
>>  				"public-key-file", sw->globals.publickeyfname);
>>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
>>  				"aes-key-file", sw->globals.aeskeyfname);
>> +#ifdef CONFIG_UBIATTACH
>>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
>>  				"mtd-blacklist", sw->globals.mtdblacklist);
>> +#endif
>>  	GET_FIELD_STRING(LIBCFG_PARSER, elem,
>>  				"postupdatecmd", sw->globals.postupdatecmd);
>>  	get_field(LIBCFG_PARSER, elem, "verbose", &sw->globals.verbose);
>> @@ -667,7 +669,7 @@ int main(int argc, char **argv)
>>  		case 'v':
>>  			loglevel = TRACELEVEL;
>>  			break;
>> -#ifdef CONFIG_MTD
>> +#ifdef CONFIG_UBIATTACH
>>  		case 'b':
>>  			mtd_set_ubiblacklist(optarg);
>>  			break;
>> @@ -863,7 +865,7 @@ int main(int argc, char **argv)
>>  		}
>>  	}
>>  
>> -#ifdef CONFIG_MTD
>> +#ifdef CONFIG_UBIATTACH
>>  	if (strlen(swcfg.globals.mtdblacklist))
>>  		mtd_set_ubiblacklist(swcfg.globals.mtdblacklist);
>>  #endif
>> diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
>> index fa59383..75fc4fb 100644
>> --- a/doc/source/swupdate.rst
>> +++ b/doc/source/swupdate.rst
>> @@ -472,7 +472,7 @@ Command line parameters
>>  +=============+==========+============================================+
>>  | -f <file>   | string   | SWUpdate config file to use                |
>>  +-------------+----------+--------------------------------------------+
>> -| -b <string> | string   | Active only if CONFIG_MTD is set           |
>> +| -b <string> | string   | Active only if CONFIG_UBIATTACH is set     |
>>  |             |          | It allows to blacklist MTDs when SWUpdate  |
>>  |             |          | searches for UBI volumes.                  |
>>  |             |          | Example: U-Boot and environment in MTD0-1: |
>> diff --git a/include/swupdate.h b/include/swupdate.h
>> index 741d24c..83e8d3a 100644
>> --- a/include/swupdate.h
>> +++ b/include/swupdate.h
>> @@ -103,7 +103,9 @@ enum {
>>  
>>  struct swupdate_global_cfg {
>>  	int verbose;
>> +#ifdef CONFIG_UBIATTACH
>>  	char mtdblacklist[SWUPDATE_GENERAL_STRING_SIZE];
>> +#endif
>>  	int loglevel;
>>  	int syslog_enabled;
>>  	int dry_run;
>>
> 
> Reviewed-by: Stefano Babic <sbbic@denx.de>
> 
> Best regards,
> Stefano Babic
> 
>