Patchwork Change programmer selection in cli and generic code

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2011-09-04 00:33:38
Message ID <4E62C762.6060904@gmx.net>
Download mbox | patch
Permalink /patch/3400/
State Superseded
Headers show

Comments

Carl-Daniel Hailfinger - 2011-09-04 00:33:38
Change programmer selection in cli and generic code

Bugfix: Do not accept multiple conflicting --programmer selections.
Restriction: Do not accept multiple --programmer selections even if
there is no conflict.
Do not rely on exported programmer variable anymore.
programmer_init requires the programmer as first parameter.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Stefan Tauner - 2011-09-04 11:03:21
On Sun, 04 Sep 2011 02:33:38 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> Change programmer selection in cli and generic code
> 
> Bugfix: Do not accept multiple conflicting --programmer selections.
> Restriction: Do not accept multiple --programmer selections even if
> there is no conflict.
> Do not rely on exported programmer variable anymore.
> programmer_init requires the programmer as first parameter.
> 
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
> 
> Index: flashrom-programmer_selection_fix/cli_classic.c
> ===================================================================
> --- flashrom-programmer_selection_fix/cli_classic.c	(Revision 1427)
> +++ flashrom-programmer_selection_fix/cli_classic.c	(Arbeitskopie)
> @@ -111,6 +111,7 @@
>  #endif
>  	int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
>  	int dont_verify_it = 0, list_supported = 0, operation_specified = 0;
> +	enum programmer prog = PROGRAMMER_INVALID;
>  	int ret = 0;
>  
>  	static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
> @@ -258,8 +259,16 @@
>  #endif
>  			break;
>  		case 'p':
> -			for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) {
> -				name = programmer_table[programmer].name;
> +			if (prog != PROGRAMMER_INVALID) {
> +				fprintf(stderr, "Error: --programmer specified "
> +					"more than once. You can specify "
> +					"multiple progammer parameters with "
> +					"\",\". Please see the man page for "

- You can specify multiple progammer parameters with ",".
+ You can specify multiple parameters for a progammer separated with ",".
or similar... the above one sounds like multiple programmers are
supported somehow imo.

> +					"details.\n");
> +				cli_classic_abort_usage();
> +			}
> +			for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {
> +				name = programmer_table[prog].name;
>  				namelen = strlen(name);
>  				if (strncmp(optarg, name, namelen) == 0) {
>  					switch (optarg[namelen]) {
> @@ -283,7 +292,7 @@
>  					break;
>  				}
>  			}
> -			if (programmer == PROGRAMMER_INVALID) {
> +			if (prog == PROGRAMMER_INVALID) {
>  				fprintf(stderr, "Error: Unknown programmer "
>  					"%s.\n", optarg);
>  				cli_classic_abort_usage();
> @@ -332,14 +341,6 @@
>  	}
>  #endif
>  
> -#if CONFIG_INTERNAL == 1
> -	if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
> -		fprintf(stderr, "Error: --mainboard requires the internal "
> -				"programmer. Aborting.\n");
> -		cli_classic_abort_usage();
> -	}
> -#endif
> -
>  	if (chip_to_probe) {
>  		for (flash = flashchips; flash && flash->name; flash++)
>  			if (!strcmp(flash->name, chip_to_probe))
> @@ -355,10 +356,21 @@
>  		flash = NULL;
>  	}
>  
> +	if (prog == PROGRAMMER_INVALID)
> +		prog = default_programmer;
> +
> +#if CONFIG_INTERNAL == 1
> +	if ((prog != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
> +		fprintf(stderr, "Error: --mainboard requires the internal "
> +				"programmer. Aborting.\n");
> +		cli_classic_abort_usage();
> +	}
> +#endif
> +
>  	/* FIXME: Delay calibration should happen in programmer code. */
>  	myusec_calibrate_delay();
>  
> -	if (programmer_init(pparam)) {
> +	if (programmer_init(prog, pparam)) {
>  		fprintf(stderr, "Error: Programmer initialization failed.\n");
>  		ret = 1;
>  		goto out_shutdown;
> Index: flashrom-programmer_selection_fix/flashrom.c
> ===================================================================
> --- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
> +++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
> @@ -42,10 +42,12 @@
>  char *chip_to_probe = NULL;
>  int verbose = 0;
>  
> +enum programmer programmer = PROGRAMMER_INVALID;
> +

everything below (i.e. the declaration and definition of
default_programmer) should be moved to the cli file(s).
selecting the default programmer is in the scope of the library user.
and it slashes away one global.

this may make chromiumos cry a bit though.

if it is moved, these ifdefs could go inline into main()... i do not
suggest this, just mentioning. a static variable is probably better.

>  #if CONFIG_INTERNAL == 1
> -enum programmer programmer = PROGRAMMER_INTERNAL;
> +enum programmer default_programmer = PROGRAMMER_INTERNAL;
>  #elif CONFIG_DUMMY == 1
> -enum programmer programmer = PROGRAMMER_DUMMY;
> +enum programmer default_programmer = PROGRAMMER_DUMMY;
>  #else
>  /* If neither internal nor dummy are selected, we must pick a sensible default.
>   * Since there is no reason to prefer a particular external programmer, we fail
> @@ -55,7 +57,7 @@
>  #if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1

way too much convenience for a (non-existent?) theoretical package
maintainer at the cost of really existing library maintainers. i would
remove the monster if and check for a compiler flag DEFAULT_PROGRAMMER
or so first. if that's not defined, look for internal and dummy as
fall-back then and just bail out with an error similar to the one below
(but with mentioning DEFAULT_PROGRAMMER).

this would also have benefits for the packager: he could select a
default programmer no matter what other programmers are enabled.

>  #error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
>  #endif
> -enum programmer programmer =
> +enum programmer default_programmer =
>  #if CONFIG_NIC3COM == 1
>  	PROGRAMMER_NIC3COM
>  #endif
> @@ -515,9 +517,15 @@
>  	return 0;
>  }
>  
> -int programmer_init(char *param)
> +int programmer_init(enum programmer prog, char *param)
>  {
>  	int ret;
> +
> +	if (prog >= PROGRAMMER_INVALID) {
> +		msg_perr("Invalid programmer specified!\n");
> +		return -1;
> +	}
> +	programmer = prog;
>  	/* Initialize all programmer specific data. */
>  	/* Default to unlimited decode sizes. */
>  	max_rom_decode = (const struct decode_sizes) {
> Index: flashrom-programmer_selection_fix/programmer.h
> ===================================================================
> --- flashrom-programmer_selection_fix/programmer.h	(Revision 1427)
> +++ flashrom-programmer_selection_fix/programmer.h	(Arbeitskopie)
> @@ -86,6 +86,7 @@
>  };
>  
>  extern enum programmer programmer;
> +extern enum programmer default_programmer;

to be removed, see above

>  struct programmer_entry {
>  	const char *vendor;
> @@ -110,7 +111,7 @@
>  
>  extern const struct programmer_entry programmer_table[];
>  
> -int programmer_init(char *param);
> +int programmer_init(enum programmer prog, char *param);
>  int programmer_shutdown(void);
>  
>  enum bitbang_spi_master_type {
> 
>
Bernd Blaauw - 2011-09-04 11:15:49
Op 4-9-2011 13:03, Stefan Tauner schreef:
>> +					"multiple progammer parameters with "
>> +					"\",\". Please see the man page for "
>
> - You can specify multiple progammer parameters with ",".
> + You can specify multiple parameters for a progammer separated with ",".

prog*R*ammer?

sorry, nothing else to add here for now.
Stefan Tauner - 2011-09-04 12:11:10
On Sun, 04 Sep 2011 13:15:49 +0200
Bernd Blaauw <bblaauw@home.nl> wrote:

> Op 4-9-2011 13:03, Stefan Tauner schreef:
> >> +					"multiple progammer parameters with "
> >> +					"\",\". Please see the man page for "
> >
> > - You can specify multiple progammer parameters with ",".
> > + You can specify multiple parameters for a progammer separated with ",".
> 
> prog*R*ammer?

:D
overlooking such things even when editing around them is creepy. thanks!

Patch

Index: flashrom-programmer_selection_fix/cli_classic.c
===================================================================
--- flashrom-programmer_selection_fix/cli_classic.c	(Revision 1427)
+++ flashrom-programmer_selection_fix/cli_classic.c	(Arbeitskopie)
@@ -111,6 +111,7 @@ 
 #endif
 	int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
 	int dont_verify_it = 0, list_supported = 0, operation_specified = 0;
+	enum programmer prog = PROGRAMMER_INVALID;
 	int ret = 0;
 
 	static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh";
@@ -258,8 +259,16 @@ 
 #endif
 			break;
 		case 'p':
-			for (programmer = 0; programmer < PROGRAMMER_INVALID; programmer++) {
-				name = programmer_table[programmer].name;
+			if (prog != PROGRAMMER_INVALID) {
+				fprintf(stderr, "Error: --programmer specified "
+					"more than once. You can specify "
+					"multiple progammer parameters with "
+					"\",\". Please see the man page for "
+					"details.\n");
+				cli_classic_abort_usage();
+			}
+			for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {
+				name = programmer_table[prog].name;
 				namelen = strlen(name);
 				if (strncmp(optarg, name, namelen) == 0) {
 					switch (optarg[namelen]) {
@@ -283,7 +292,7 @@ 
 					break;
 				}
 			}
-			if (programmer == PROGRAMMER_INVALID) {
+			if (prog == PROGRAMMER_INVALID) {
 				fprintf(stderr, "Error: Unknown programmer "
 					"%s.\n", optarg);
 				cli_classic_abort_usage();
@@ -332,14 +341,6 @@ 
 	}
 #endif
 
-#if CONFIG_INTERNAL == 1
-	if ((programmer != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
-		fprintf(stderr, "Error: --mainboard requires the internal "
-				"programmer. Aborting.\n");
-		cli_classic_abort_usage();
-	}
-#endif
-
 	if (chip_to_probe) {
 		for (flash = flashchips; flash && flash->name; flash++)
 			if (!strcmp(flash->name, chip_to_probe))
@@ -355,10 +356,21 @@ 
 		flash = NULL;
 	}
 
+	if (prog == PROGRAMMER_INVALID)
+		prog = default_programmer;
+
+#if CONFIG_INTERNAL == 1
+	if ((prog != PROGRAMMER_INTERNAL) && (lb_part || lb_vendor)) {
+		fprintf(stderr, "Error: --mainboard requires the internal "
+				"programmer. Aborting.\n");
+		cli_classic_abort_usage();
+	}
+#endif
+
 	/* FIXME: Delay calibration should happen in programmer code. */
 	myusec_calibrate_delay();
 
-	if (programmer_init(pparam)) {
+	if (programmer_init(prog, pparam)) {
 		fprintf(stderr, "Error: Programmer initialization failed.\n");
 		ret = 1;
 		goto out_shutdown;
Index: flashrom-programmer_selection_fix/flashrom.c
===================================================================
--- flashrom-programmer_selection_fix/flashrom.c	(Revision 1427)
+++ flashrom-programmer_selection_fix/flashrom.c	(Arbeitskopie)
@@ -42,10 +42,12 @@ 
 char *chip_to_probe = NULL;
 int verbose = 0;
 
+enum programmer programmer = PROGRAMMER_INVALID;
+
 #if CONFIG_INTERNAL == 1
-enum programmer programmer = PROGRAMMER_INTERNAL;
+enum programmer default_programmer = PROGRAMMER_INTERNAL;
 #elif CONFIG_DUMMY == 1
-enum programmer programmer = PROGRAMMER_DUMMY;
+enum programmer default_programmer = PROGRAMMER_DUMMY;
 #else
 /* If neither internal nor dummy are selected, we must pick a sensible default.
  * Since there is no reason to prefer a particular external programmer, we fail
@@ -55,7 +57,7 @@ 
 #if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_FT2232_SPI+CONFIG_SERPROG+CONFIG_BUSPIRATE_SPI+CONFIG_DEDIPROG+CONFIG_RAYER_SPI+CONFIG_NICINTEL+CONFIG_NICINTEL_SPI+CONFIG_OGP_SPI+CONFIG_SATAMV > 1
 #error Please enable either CONFIG_DUMMY or CONFIG_INTERNAL or disable support for all programmers except one.
 #endif
-enum programmer programmer =
+enum programmer default_programmer =
 #if CONFIG_NIC3COM == 1
 	PROGRAMMER_NIC3COM
 #endif
@@ -515,9 +517,15 @@ 
 	return 0;
 }
 
-int programmer_init(char *param)
+int programmer_init(enum programmer prog, char *param)
 {
 	int ret;
+
+	if (prog >= PROGRAMMER_INVALID) {
+		msg_perr("Invalid programmer specified!\n");
+		return -1;
+	}
+	programmer = prog;
 	/* Initialize all programmer specific data. */
 	/* Default to unlimited decode sizes. */
 	max_rom_decode = (const struct decode_sizes) {
Index: flashrom-programmer_selection_fix/programmer.h
===================================================================
--- flashrom-programmer_selection_fix/programmer.h	(Revision 1427)
+++ flashrom-programmer_selection_fix/programmer.h	(Arbeitskopie)
@@ -86,6 +86,7 @@ 
 };
 
 extern enum programmer programmer;
+extern enum programmer default_programmer;
 
 struct programmer_entry {
 	const char *vendor;
@@ -110,7 +111,7 @@ 
 
 extern const struct programmer_entry programmer_table[];
 
-int programmer_init(char *param);
+int programmer_init(enum programmer prog, char *param);
 int programmer_shutdown(void);
 
 enum bitbang_spi_master_type {