Make all programmer/CLI options case-insensitive

Submitter	Uwe Hermann
Date	2011-07-29 12:55:49
Message ID	<20110729125549.GS4802@greenwood>
Download	mbox \| patch
Permalink	/patch/3316/
State	Bitrotted
Headers	show Return-path: <flashrom-bounces@flashrom.org> Envelope-to: patchwork@coreboot.org Date: Fri, 29 Jul 2011 14:55:49 +0200 From: Uwe Hermann <uwe@hermann-uwe.de> To: flashrom@flashrom.org Message-ID: <20110729125549.GS4802@greenwood> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="6BvahUXLYAruDZOj" Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Subject: [flashrom] [PATCH] Make all programmer/CLI options case-insensitive Precedence: list Errors-To: flashrom-bounces@flashrom.org Sender: flashrom-bounces@flashrom.org

Comments

Uwe Hermann - 2011-07-29 12:55:49

See patch.


Uwe.

Stefan Tauner - 2011-07-29 13:25:37

On Fri, 29 Jul 2011 14:55:49 +0200
Uwe Hermann <uwe@hermann-uwe.de> wrote:

> There's no need to make life for our user unnecessarily hard
not all cases may be *unnecessarily* hard. the
laptop=force_I_want_a_brick parameter might be such a case where we
really want the user to spend some time before getting it to work :)
this is not a NAK at all. basically i think it is a good idea.

Carl-Daniel Hailfinger - 2011-07-29 21:18:06

Am 29.07.2011 14:55 schrieb Uwe Hermann:
> Make all programmer/CLI options case-insensitive.
>
> There's no need to make life for our user unnecessarily hard, and we already
> have many case-insensitive options where strcasecmp() is used.
>
> Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>
>   

I'm not sure this is a good idea. Our options/commands are
case-sensitive (-V is totally different from -v), and making some
parameters case-insensitive might lead people to assume that all
parameters are case-insensitive.

Regards,
Carl-Daniel

Uwe Hermann - 2011-08-25 21:45:47

On Fri, Jul 29, 2011 at 11:18:06PM +0200, Carl-Daniel Hailfinger wrote:
> Am 29.07.2011 14:55 schrieb Uwe Hermann:
> > Make all programmer/CLI options case-insensitive.
> >
> > There's no need to make life for our user unnecessarily hard, and we already
> > have many case-insensitive options where strcasecmp() is used.
> >
> > Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>
> >   
> 
> I'm not sure this is a good idea. Our options/commands are
> case-sensitive (-V is totally different from -v), and making some
> parameters case-insensitive might lead people to assume that all
> parameters are case-insensitive.

I don't think that's an issue, it's probably pretty clear to most people
using CLI tools that options are almost always case-sensitive, but parameters
don't have to be. We can also additionally mention that in the manpage.

The behaviour should definately be consistent for the tool, though, even
if that means we convert everything to case-sensitive (though I'd much
prefer case-insensitive).

It's very inconsistent for the tool to allow both
  $ flashrom -p ft2232_spi:type=jtagkey,port=A
or
  $ flashrom -p ft2232_spi:type=jTAgkEy,port=A

Whereas we explicitly force the user to use

  $ flashrom -p internal:boardenable=force
and these will not work:
  $ flashrom -p internal:boardenable=Force
  $ flashrom -p internal:boardenable=FORCE

And also the user is forced to type/use:
  $ flashrom -p dummy:emulate=SST25VF032B
whereas 
  $ flashrom -p dummy:emulate=sst25vf032b
will not work.

As Stefan said, force_I_want_a_brick may be the only exception (i.e., it
could really be case-sensitive) as we explicitly want to make it harder
for the user to type the string in this case.

Uwe.

Carl-Daniel Hailfinger - 2011-09-02 17:51:28

Am 25.08.2011 23:45 schrieb Uwe Hermann:
> On Fri, Jul 29, 2011 at 11:18:06PM +0200, Carl-Daniel Hailfinger wrote:
>   
>> Am 29.07.2011 14:55 schrieb Uwe Hermann:
>>     
>>> Make all programmer/CLI options case-insensitive.
>>>
>>> There's no need to make life for our user unnecessarily hard, and we already
>>> have many case-insensitive options where strcasecmp() is used.
>>>
>>> Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>
>>>   
>>>       
>> I'm not sure this is a good idea. Our options/commands are
>> case-sensitive (-V is totally different from -v), and making some
>> parameters case-insensitive might lead people to assume that all
>> parameters are case-insensitive.
>>     
> I don't think that's an issue, it's probably pretty clear to most people
> using CLI tools that options are almost always case-sensitive, but parameters
> don't have to be. We can also additionally mention that in the manpage.
>
> The behaviour should definately be consistent for the tool, though, even
> if that means we convert everything to case-sensitive (though I'd much
> prefer case-insensitive).
>
> It's very inconsistent for the tool to allow both
>   $ flashrom -p ft2232_spi:type=jtagkey,port=A
> or
>   $ flashrom -p ft2232_spi:type=jTAgkEy,port=A
>
> Whereas we explicitly force the user to use
>
>   $ flashrom -p internal:boardenable=force
> and these will not work:
>   $ flashrom -p internal:boardenable=Force
>   $ flashrom -p internal:boardenable=FORCE
>
> And also the user is forced to type/use:
>   $ flashrom -p dummy:emulate=SST25VF032B
> whereas 
>   $ flashrom -p dummy:emulate=sst25vf032b
> will not work.
>
>
> As Stefan said, force_I_want_a_brick may be the only exception (i.e., it
> could really be case-sensitive) as we explicitly want to make it harder
> for the user to type the string in this case.
>   

Mh. Three requests:
- don't document the case insensitive matching
- force_I_want_a_brick keeps case sensitivity
- think long and hard about supportability of the new interface for
later flashrom versions.

If Stefan Tauner thinks that's OK, I retract my objection.

Regards,
Carl-Daniel

Patch

Make all programmer/CLI options case-insensitive.

There's no need to make life for our user unnecessarily hard, and we already
have many case-insensitive options where strcasecmp() is used.

Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>

Index: dummyflasher.c
===================================================================
--- dummyflasher.c	(Revision 1399)
+++ dummyflasher.c	(Arbeitskopie)
@@ -147,7 +147,7 @@ 
 		goto dummy_init_out;
 	}
 #if EMULATE_SPI_CHIP
-	if (!strcmp(tmp, "M25P10.RES")) {
+	if (!strcasecmp(tmp, "M25P10.RES")) {
 		emu_chip = EMULATE_ST_M25P10_RES;
 		emu_chip_size = 128 * 1024;
 		emu_max_byteprogram_size = 128;
@@ -160,7 +160,7 @@ 
 		msg_pdbg("Emulating ST M25P10.RES SPI flash chip (RES, page "
 			 "write)\n");
 	}
-	if (!strcmp(tmp, "SST25VF040.REMS")) {
+	if (!strcasecmp(tmp, "SST25VF040.REMS")) {
 		emu_chip = EMULATE_SST_SST25VF040_REMS;
 		emu_chip_size = 512 * 1024;
 		emu_max_byteprogram_size = 1;
@@ -173,7 +173,7 @@ 
 		msg_pdbg("Emulating SST SST25VF040.REMS SPI flash chip (REMS, "
 			 "byte write)\n");
 	}
-	if (!strcmp(tmp, "SST25VF032B")) {
+	if (!strcasecmp(tmp, "SST25VF032B")) {
 		emu_chip = EMULATE_SST_SST25VF032B;
 		emu_chip_size = 4 * 1024 * 1024;
 		emu_max_byteprogram_size = 1;
Index: cli_classic.c
===================================================================
--- cli_classic.c	(Revision 1399)
+++ cli_classic.c	(Arbeitskopie)
@@ -341,7 +341,7 @@ 
 
 	if (chip_to_probe) {
 		for (flash = flashchips; flash && flash->name; flash++)
-			if (!strcmp(flash->name, chip_to_probe))
+			if (!strcasecmp(flash->name, chip_to_probe))
 				break;
 		if (!flash || !flash->name) {
 			fprintf(stderr, "Error: Unknown chip '%s' specified.\n",
Index: internal.c
===================================================================
--- internal.c	(Revision 1399)
+++ internal.c	(Arbeitskopie)
@@ -142,7 +142,7 @@ 
 	char *arg;
 
 	arg = extract_programmer_param("boardenable");
-	if (arg && !strcmp(arg,"force")) {
+	if (arg && !strcasecmp(arg, "force")) {
 		force_boardenable = 1;
 	} else if (arg && !strlen(arg)) {
 		msg_perr("Missing argument for boardenable.\n");
@@ -156,7 +156,7 @@ 
 	free(arg);
 
 	arg = extract_programmer_param("boardmismatch");
-	if (arg && !strcmp(arg,"force")) {
+	if (arg && !strcasecmp(arg, "force")) {
 		force_boardmismatch = 1;
 	} else if (arg && !strlen(arg)) {
 		msg_perr("Missing argument for boardmismatch.\n");
@@ -170,7 +170,7 @@ 
 	free(arg);
 
 	arg = extract_programmer_param("laptop");
-	if (arg && !strcmp(arg,"force_I_want_a_brick")) {
+	if (arg && !strcasecmp(arg, "force_I_want_a_brick")) {
 		force_laptop = 1;
 	} else if (arg && !strlen(arg)) {
 		msg_perr("Missing argument for laptop.\n");
Index: flashrom.8
===================================================================
--- flashrom.8	(Revision 1399)
+++ flashrom.8	(Arbeitskopie)
@@ -85,8 +85,7 @@ 
 Probe only for the specified flash ROM chip. This option takes the chip name as
 printed by
 .B "flashrom \-L"
-without the vendor name as parameter. Please note that the chip name is
-case sensitive.
+without the vendor name as parameter.
 .TP
 .B "\-m, \-\-mainboard" [<vendor>:]<board>
 Override mainboard settings.
Index: w29ee011.c
===================================================================
--- w29ee011.c	(Revision 1399)
+++ w29ee011.c	(Arbeitskopie)
@@ -30,7 +30,7 @@ 
 	chipaddr bios = flash->virtual_memory;
 	uint8_t id1, id2;
 
-	if (!chip_to_probe || strcmp(chip_to_probe, flash->name)) {
+	if (!chip_to_probe || strcasecmp(chip_to_probe, flash->name)) {
 		msg_cdbg("Old Winbond W29* probe method disabled because "
 			 "the probing sequence puts the AMIC A49LF040A in "
 			 "a funky state. Use 'flashrom -c %s' if you "
Index: flashrom.c
===================================================================
--- flashrom.c	(Revision 1399)
+++ flashrom.c	(Arbeitskopie)
@@ -1135,7 +1135,7 @@ 
 	char *tmp;
 
 	for (flash = flashchips + startchip; flash && flash->name; flash++) {
-		if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0)
+		if (chip_to_probe && strcasecmp(flash->name, chip_to_probe) != 0)
 			continue;
 		buses_common = buses_supported & flash->bustype;
 		if (!buses_common) {

Patchwork Make all programmer/CLI options case-insensitive

Comments

Patch