Re: [flashrom] [PATCH] flashrom 0.9.2

[Michael Karcher]

Am Samstag, den 15.05.2010, 14:49 +0200 schrieb Carl-Daniel Hailfinger:
> > >       /* FIXME: --mainboard should be a programmer parameter */
> >
> > Right.
>
> Post 0.9.2, I'd say.

Yes, definitely.
> > >       "   -n | --noverify                   don't auto-verify\n"
> > >       "   -l | --layout <file>              read ROM layout from "
> > >       "file\n"
> >
> > Style: I would indent "file" up to "read ROM layout". Your choice.
>
> It's only for making the code conform to 80 columns. The output is in
> one line. As such, I'd like to have the string in the code start at the
> beginning to make that painfully obvious.

Point taken. As discussed on IRC, indent by two characters to make even
more clear that the previous line does not end in "\n".
> > > +#if 0
> > > /* FIXME: This is broken. printf_debug is a no-op because verbose=0
> > > * at this point.
> > > */
> > > if (argc > 1) {
> > >   /* Yes, print them. */
> > >   printf_debug("The arguments are:\n");
> > >   for (i = 1; i < argc; ++i)
> > >   	printf_debug("%s\n", argv[i]);
> > >  }
> > >
> > > +#endif
> >
> > Throw it out completely. Whoever just wants to see the parameters
> > flashrom is called with should write a wrapper script.
>
> I'd love to acticate that code permanently, but it does make sense to
> move the code down after option parsing, and reconstruct the arguments
> there.

Reconstructing arguments is possible, but describing what flashrom is
going to do seems more useful (also discussed on IRC). If problems are
reported about command line parsing, they should also include the
command line already.
> > > 		exit(1);
> > > 		cli_classic_abort_usage(argv[0]);
> >
> > Don't dump the usage on syntax errors. It clutters output too much so
> > that you don't find the error message. Just print a one-liner like 'run
> > "flashrom --help" for usage info'. I especially liked the debian tool
> > that outputs "You need --help.", but don't recommend this very phrase to
> > flashrom.
>
> I've changed cli_classic_abort_usage accordingly.

Fine.
> > > 	if (programmer == PROGRAMMER_INVALID) {
> > > 		printf("Error: Unknown programmer %s.\n", optarg);
> > > 		exit(1);
> > > 		cli_classic_abort_usage(argv[0]);
> > > }
> >
> > In this case, list the supported programmers (and just them).
>
> Hm. My current version tells the user to run "flashrom --help" and that
> will print a list of supported programmers. Is that OK for you?

Fine for now. But I still like the programmer list directly here better.
> > > msg_cinfo("VERIFIED.          \n");
> > > msg_cinfo("VERIFIED.\n");
> >
> > The spaces probably were to clear address counter characters. Did you
> > verify that it is not needed with current chip drivers?
>
> Ouch, right! Thanks for spotting this. (One of the reasons I dislike the
> current progress printing stuff.)

Yeah, that part of flashrom is  bit hacky.
> Thanks for the review, could you please check if the new patch is OK?
> I added an error if --mainboard is specified for a non-internal
> programmer, fixed wiki printing compilation and adjusted some messages.
> printf("Please note that the command line interface for flashrom has "
>       "changed between\n"
>       "0.9.1 and 0.9.2 and will change again before flashrom 1.0.\n"
>       "Do not use flashrom in scripts or other automated tools "
>       "without checking\n"
>       "that your flashrom version won't interpret options in a "
>       "different way.\n\n");

You are right. Indentation style (two characters indented for
continuation lines) makes senses here, too.
> /* FIXME: This message is designed towards CLI users. */
> msg_cinfo("Please email a report to [e-mail] if any "
>        "of the above operations\nwork correctly for you with "
>        "this flash part. Please include the flashrom\noutput "
>        "with the additional -V option for all operations you "
>        "tested (-V, -rV,\n-wV, -EV), and mention which "
>        "mainboard or programmer you tested.\nThanks for your "
>        "help!\n===\n");
> msg_cinfo("The test status of this chip may have been updated "
> 	  "in the latest development\n"
> 	  "version of flashrom. If you are running the latest "
> 	  "development version,\n"
> 	  "please email a report to [e-mail] if "
> 	  "any of the above operations\n"
> 	  "work correctly for you with this flash part. Please "
> 	  "include the flashrom\n"
> 	  "output with the additional -V option for all "
> 	  "operations you tested (-V, -Vr,\n"
> 	  "-Vw, -VE), and mention which mainboard or "
> 	  "programmer you tested.\n"
> 	  "Thanks for your help!\n===\n");
> }

And here.

Whether you change the indentation or not:
Acked-by: Michael Karcher

Regards,
  Michael Karcher