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 |
Comments
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.
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
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.
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) {