Re: [flashrom] [PATCH] Make the laptop warning less scary if unsure

[Carl-Daniel Hailfinger]

Am 20.08.2011 15:14 schrieb Stefan Tauner:
> Telling the user to use "force_I_want_a_brick" if it is not even a
> laptop, is a bit over-the-top. Introduce a new laptop parameter
> "this_is_not_a_laptop" that allows to force operation, but only if
> the detection is not sure.
>
> ---
>
> complicates the whole thing a bit. could do without the extra
> variable and new part of the if condition, if we allow the
> this_is_not_a_laptop parameter to always work, not only if the
> laptop detection is uncertain. it will be undocumented and only
> visible to the user together with the warning anyway...
>
> Signed-off-by: Stefan Tauner <[e-mail]>
> ---
>  internal.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/internal.c b/internal.c
> index 846cf1e..5ff605e 100644
> --- a/internal.c
> +++ b/internal.c
> @@ -170,9 +171,11 @@ int internal_init(void)
>   free(arg);
>
> arg = extract_programmer_param("laptop");
> -	if (arg && !strcmp(arg,"force_I_want_a_brick")) {
> +	if (arg && !strcmp(arg,"force_I_want_a_brick"))

While you're touching that line, can you add a space after the comma?
> force_laptop = 1;
> -	} else if (arg && !strlen(arg)) {
> +	else if (arg && !strcmp(arg,"this_is_not_a_laptop"))

Same here.
> +		not_a_laptop = 1;
> +	else if (arg && !strlen(arg)) {
>       msg_perr("Missing argument for laptop.\n");
>       free(arg);
>       return 1;
> @@ -251,9 +254,8 @@ int internal_init(void)
>       	 "You have been warned.\n"
>       	 "========================================================================\n");
>
> -		if (force_laptop) {
> -			msg_perr("Proceeding anyway because user specified "
> -				 "laptop=force_I_want_a_brick\n");
> +		if (force_laptop || (not_a_laptop && (is_laptop != 1))) {

What about is_laptop == 2 instead?
> +			msg_perr("Proceeding anyway because user forced us to.\n");
>       } else {
>       	msg_perr("Aborting.\n");
>       	exit(1);

Acked-by: Carl-Daniel Hailfinger <[e-mail]>

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH] Make the laptop warning less scary if unsure

[Stefan Tauner]

On Wed, 14 Sep 2011 00:15:40 +0200
Carl-Daniel Hailfinger <[e-mail]> wrote:
> Am 20.08.2011 15:14 schrieb Stefan Tauner:
> > Telling the user to use "force_I_want_a_brick" if it is not even a
> > laptop, is a bit over-the-top. Introduce a new laptop parameter
> > "this_is_not_a_laptop" that allows to force operation, but only if
> > the detection is not sure.
> >
> > ---
> >
> > complicates the whole thing a bit. could do without the extra
> > variable and new part of the if condition, if we allow the
> > this_is_not_a_laptop parameter to always work, not only if the
> > laptop detection is uncertain. it will be undocumented and only
> > visible to the user together with the warning anyway...
> >
> > Signed-off-by: Stefan Tauner <[e-mail]>
> > ---
> >  internal.c |   14 ++++++++------
> >  1 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/internal.c b/internal.c
> > index 846cf1e..5ff605e 100644
> > --- a/internal.c
> > +++ b/internal.c
> > @@ -170,9 +171,11 @@ int internal_init(void)
> >   free(arg);
> >
> > arg = extract_programmer_param("laptop");
> > -	if (arg && !strcmp(arg,"force_I_want_a_brick")) {
> > +	if (arg && !strcmp(arg,"force_I_want_a_brick"))
>
> While you're touching that line, can you add a space after the comma?
> > force_laptop = 1;
> > -	} else if (arg && !strlen(arg)) {
> > +	else if (arg && !strcmp(arg,"this_is_not_a_laptop"))
>
> Same here.
> > +		not_a_laptop = 1;
> > +	else if (arg && !strlen(arg)) {
> >       msg_perr("Missing argument for laptop.\n");
> >       free(arg);
> >       return 1;
> > @@ -251,9 +254,8 @@ int internal_init(void)
> >       	 "You have been warned.\n"
> >       	 "========================================================================\n");
> >
> > -		if (force_laptop) {
> > -			msg_perr("Proceeding anyway because user specified "
> > -				 "laptop=force_I_want_a_brick\n");
> > +		if (force_laptop || (not_a_laptop && (is_laptop != 1))) {
>
> What about is_laptop == 2 instead?

i think i wanted to make the opposition to is_laptop == 1 more
explicit, but now i think == 2 is more appropriate.
> > +			msg_perr("Proceeding anyway because user forced us to.\n");
> >       } else {
> >       	msg_perr("Aborting.\n");
> >       	exit(1);
>
> Acked-by: Carl-Daniel Hailfinger <[e-mail]>

thanks, committed with all suggested changes in r1440.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner