[PATCH] Abort on unused programmer parameters

[Carl-Daniel Hailfinger]

An unused programmer parameter is a sign that the user wanted to either
do something not supported by the programmer or misspelled a parameter
which may be essential for the given programmer. Aborting is the only
safe choice.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-abort_unused_programmer_param/flashrom.c
===================================================================
--- flashrom-abort_unused_programmer_param/flashrom.c	(Revision 1706)
+++ flashrom-abort_unused_programmer_param/flashrom.c	(Arbeitskopie)
@@ -389,13 +389,14 @@
    programmer_may_write = 1;

programmer_param = param;
-	msg_pdbg("Initializing %s programmer\n",
-		 programmer_table[programmer].name);
+	msg_pdbg("Initializing %s programmer\n", programmer_table[programmer].name);
    ret = programmer_table[programmer].init();
    if (programmer_param && strlen(programmer_param)) {
-		msg_perr("Unhandled programmer parameters: %s\n",
-			 programmer_param);
-		/* Do not error out here, the init itself was successful. */
+		msg_perr("Unhandled programmer parameters: %s\n", programmer_param);
+		msg_perr("Aborting.\n");
+		/* Do not overwrite any error code from programmer init. */
+		if (!ret)
+			ret = ERROR_FATAL;
    }
    return ret;
 }

[Stefan Tauner]

On Sat, 10 Aug 2013 17:43:15 +0200
Carl-Daniel Hailfinger  wrote:
> An unused programmer parameter is a sign that the user wanted to either
> do something not supported by the programmer or misspelled a parameter
> which may be essential for the given programmer. Aborting is the only
> safe choice.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-abort_unused_programmer_param/flashrom.c
>
> --- flashrom-abort_unused_programmer_param/flashrom.c	(Revision 1706)
> +++ flashrom-abort_unused_programmer_param/flashrom.c	(Arbeitskopie)
> @@ -389,13 +389,14 @@
>    programmer_may_write = 1;
>
> programmer_param = param;
> msg_pdbg("Initializing %s programmer\n",
>  programmer_table[programmer].name);
> msg_pdbg("Initializing %s programmer\n", programmer_table[programmer].name);
> ret = programmer_table[programmer].init();
> if (programmer_param && strlen(programmer_param)) {
> msg_perr("Unhandled programmer parameters: %s\n",
> 	 programmer_param);
> /* Do not error out here, the init itself was successful. */
> msg_perr("Unhandled programmer parameters: %s\n", programmer_param);
> msg_perr("Aborting.\n");

Considering pro and contra arguments for (not) combining these two
lines, I am fine with it.</blah :)>
> /* Do not overwrite any error code from programmer init. */
> if (!ret)
> 	ret = ERROR_FATAL;
> }
> return ret;
>
> }

IMHO the if is unnecessary, not beneficial and should be gone.
In any case it is 
Acked-by: Stefan Tauner 
and I want that in 0.9.7.

[Carl-Daniel Hailfinger]

Am 12.08.2013 00:53 schrieb Stefan Tauner:
> On Sat, 10 Aug 2013 17:43:15 +0200
> Carl-Daniel Hailfinger  wrote:
> > An unused programmer parameter is a sign that the user wanted to either
> > do something not supported by the programmer or misspelled a parameter
> > which may be essential for the given programmer. Aborting is the only
> > safe choice.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-abort_unused_programmer_param/flashrom.c
> >
> > --- flashrom-abort_unused_programmer_param/flashrom.c	(Revision 1706)
> > +++ flashrom-abort_unused_programmer_param/flashrom.c	(Arbeitskopie)
> > @@ -389,13 +389,14 @@
> >    programmer_may_write = 1;
> >
> > programmer_param = param;
> > msg_pdbg("Initializing %s programmer\n",
> >  programmer_table[programmer].name);
> > msg_pdbg("Initializing %s programmer\n", programmer_table[programmer].name);
> > ret = programmer_table[programmer].init();
> > if (programmer_param && strlen(programmer_param)) {
> > msg_perr("Unhandled programmer parameters: %s\n",
> > 	 programmer_param);
> > /* Do not error out here, the init itself was successful. */
> > msg_perr("Unhandled programmer parameters: %s\n", programmer_param);
> > msg_perr("Aborting.\n");
>
> Considering pro and contra arguments for (not) combining these two
> lines, I am fine with it.</blah :)>
> > /* Do not overwrite any error code from programmer init. */
> > if (!ret)
> > 	ret = ERROR_FATAL;
> > }
> > return ret;
> >
> > }
>
> IMHO the if is unnecessary, not beneficial and should be gone.
> In any case it is 
> Acked-by: Stefan Tauner 
> and I want that in 0.9.7.

Thanks!

After an extensive review on IRC, this will hopefully be a proper
implementation of the agreed upon programmer behaviour.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-abort_unused_programmer_param/flashrom.c
===================================================================
--- flashrom-abort_unused_programmer_param/flashrom.c	(Revision 1706)
+++ flashrom-abort_unused_programmer_param/flashrom.c	(Arbeitskopie)
@@ -389,13 +389,23 @@
    programmer_may_write = 1;

programmer_param = param;
-	msg_pdbg("Initializing %s programmer\n",
-		 programmer_table[programmer].name);
+	msg_pdbg("Initializing %s programmer\n", programmer_table[programmer].name);
    ret = programmer_table[programmer].init();
    if (programmer_param && strlen(programmer_param)) {
-		msg_perr("Unhandled programmer parameters: %s\n",
-			 programmer_param);
-		/* Do not error out here, the init itself was successful. */
+		if (ret != 0) {
+			/* It is quite possible that any unhandled programmer parameter would have been valid,
+			 * but an error in actual programmer init happened before the parameter was evaluated.
+			 */
+			msg_pwarn("Unhandled programmer parameters (possibly due to another failure): %s\n",
+				  programmer_param);
+		} else {
+			/* Actual programmer init was successful, but the user specified an invalid or unusable
+			 * (for the current programmer configuration) parameter.
+			 */
+			msg_perr("Unhandled programmer parameters: %s\n", programmer_param);
+			msg_perr("Aborting.\n");
+			ret = ERROR_FATAL;
+		}
    }
    return ret;
 }

[Stefan Tauner]

On Tue, 13 Aug 2013 01:25:17 +0200
Carl-Daniel Hailfinger  wrote:
> Am 12.08.2013 00:53 schrieb Stefan Tauner:
> > IMHO the if is unnecessary, not beneficial and should be gone.
> > In any case it is 
> > Acked-by: Stefan Tauner 
> > and I want that in 0.9.7.
>
> Thanks!
>
> After an extensive review on IRC, this will hopefully be a proper
> implementation of the agreed upon programmer behaviour.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-abort_unused_programmer_param/flashrom.c
>
> --- flashrom-abort_unused_programmer_param/flashrom.c	(Revision 1706)
> +++ flashrom-abort_unused_programmer_param/flashrom.c	(Arbeitskopie)
> @@ -389,13 +389,23 @@
>    programmer_may_write = 1;
>
> programmer_param = param;
> msg_pdbg("Initializing %s programmer\n",
>  programmer_table[programmer].name);
> msg_pdbg("Initializing %s programmer\n", programmer_table[programmer].name);
> ret = programmer_table[programmer].init();
> if (programmer_param && strlen(programmer_param)) {
> msg_perr("Unhandled programmer parameters: %s\n",
> 	 programmer_param);
> /* Do not error out here, the init itself was successful. */
> if (ret != 0) {

I would have swapped the if/else branches, i.e. if (ret == 0) /* hard
error */ else /* possibly due to another failure */, but that's just my
weird taste probably :)
> 	/* It is quite possible that any unhandled programmer parameter would have been valid,
> 	 * but an error in actual programmer init happened before the parameter was evaluated.
> 	 */
> 	msg_pwarn("Unhandled programmer parameters (possibly due to another failure): %s\n",
> 		  programmer_param);
> } else {
> 	/* Actual programmer init was successful, but the user specified an invalid or unusable
> 	 * (for the current programmer configuration) parameter.
> 	 */
> 	msg_perr("Unhandled programmer parameters: %s\n", programmer_param);
> 	msg_perr("Aborting.\n");
> 	ret = ERROR_FATAL;
> }
> }
> return ret;

I can not imagine how this could possibly break anything in vanilla
flashrom. I have also tested it with the dummy programmer:

$ ./flashrom -p dummy:emulate=M25P10.RES,fail
flashrom v0.9.6.1-r1707 on Linux 3.8.0-6-generic (x86_64)
flashrom is free software, get the source code at http://www.flashrom.org

Calibrating delay loop... OK.
Unhandled programmer parameters: fail
Aborting.
Error: Programmer initialization failed.

$ ./flashrom -p dummy:emulate=initfail,image=persistent.img
flashrom v0.9.6.1-r1707 on Linux 3.8.0-6-generic (x86_64)
flashrom is free software, get the source code at http://www.flashrom.org

Calibrating delay loop... OK.
Invalid chip specified for emulation: initfail
Unhandled programmer parameters (possibly due to another failure): image=persistent.img
Error: Programmer initialization failed.

Hence...
Acked-by: Stefan Tauner

[Carl-Daniel Hailfinger]

Am 13.08.2013 04:30 schrieb Stefan Tauner:
> On Tue, 13 Aug 2013 01:25:17 +0200
> Carl-Daniel Hailfinger  wrote:
> > --- flashrom-abort_unused_programmer_param/flashrom.c	(Revision 1706)
> > +++ flashrom-abort_unused_programmer_param/flashrom.c	(Arbeitskopie)
> > @@ -389,13 +389,23 @@
> >    programmer_may_write = 1;
> >
> > programmer_param = param;
> > msg_pdbg("Initializing %s programmer\n",
> >  programmer_table[programmer].name);
> > msg_pdbg("Initializing %s programmer\n", programmer_table[programmer].name);
> > ret = programmer_table[programmer].init();
> > if (programmer_param && strlen(programmer_param)) {
> > msg_perr("Unhandled programmer parameters: %s\n",
> > 	 programmer_param);
> > /* Do not error out here, the init itself was successful. */
> > if (ret != 0) {
>
> I would have swapped the if/else branches, i.e. if (ret == 0) /* hard
> error */ else /* possibly due to another failure */, but that's just my
> weird taste probably :)
> I can not imagine how this could possibly break anything in vanilla
> flashrom. I have also tested it with the dummy programmer:
>
> $ ./flashrom -p dummy:emulate=M25P10.RES,fail
> flashrom v0.9.6.1-r1707 on Linux 3.8.0-6-generic (x86_64)
> flashrom is free software, get the source code at http://www.flashrom.org
>
> Calibrating delay loop... OK.
> Unhandled programmer parameters: fail
> Aborting.
> Error: Programmer initialization failed.
>
> $ ./flashrom -p dummy:emulate=initfail,image=persistent.img
> flashrom v0.9.6.1-r1707 on Linux 3.8.0-6-generic (x86_64)
> flashrom is free software, get the source code at http://www.flashrom.org
>
> Calibrating delay loop... OK.
> Invalid chip specified for emulation: initfail
> Unhandled programmer parameters (possibly due to another failure): image=persistent.img
> Error: Programmer initialization failed.
>
> Hence...
> Acked-by: Stefan Tauner

Thanks for the review and for testing!

Committed in r1708.

Regards,
Carl-Daniel