[flashrom] Using libflashrom from fwupd

[Stefan Tauner]

On Thu, 17 Aug 2017 00:53:36 +0200
Nico Huber <nico.h at gmx.de> wrote:

> 
> I guess this would suffice for read and verify, which are simple,
> continuous operations. Write, however, is comprised of one, two or three
> steps (depending on options and the data): Read (needed to know which
> bytes have to change), (comparing to the data to be written) write (if
> necessary) and optionally verify. So there is no straight forward way
> to tell during read, for instance, if we are at 100% after the read or
> not. We could compare the data early, but that might result in an awk-
> ward implementation. My idea (not yet convinced if it's any good) would
> be to add a `stage` parameter to the callback:
> 
>     enum flashrom_progress_stage {
>         FLASHROM_PROGRESS_READ,
>         FLASHROM_PROGRESS_WRITE,
>         FLASHROM_PROGRESS_VERIFY,
>     };
> 

Is there really a good reason to leak these much details to the UI? Of
course it would allow it to draw a very detailed and realistic picture
(and a dedicated flashrom UI might want to do that (possibly just for
the lols though)) but for a generic UI where libflashrom is just one of
multiple targets I don't think this is helpful at all but complicates
things.

Richard, we cannot easily determine how many net (as in netto) bytes we
have to transfer (in and out) because it very much depends on the
circumstances as Nico explained.
However, we can make pretty good guesses that get updated during the
operation and could simply pass a percentage. The progress would not be
constant in speed (e.g. because reading is usually much faster than
writing), and it would sometimes even be non-monotonic (if a write
fails, we re-read the whole chip to make absolutely sure we get
everything correct thus increasing the total number of bytes to
transfer... or after the first read we might notice that we can skip
writing 90% of the blocks because only a small part is updated) but this
would drastically reduce the complexity of the interface and would still
update often (spinner-ish) and usually relatively consistently and
monotonic. I think this is the best compromise between doing nothing
and getting insane by trying to get all information to the caller that
it needs to get a complete picture (because the latter is by definition
impossible since we don't know it either).

I personally don't like to lie to users even if they beg for it. If
flashrom does not exit it does useful stuff (of course there are some
corner cases, e.g., if hardware is broken but these cases should a) be
fixed by timeouts within flashrom, and b) be covered by timeouts in the
callers and NOT by the user manually killing anything...). So IMHO a
message to not worry + a spinner as long as your UI is working
correctly (+ a timeout that really is the last resort) is way better
and more honest than a made up progress indicator that can never tell
the full story... but that's my subjective opinion. flashrom as of yet
does not even have the spinner (and that's bad!) and to add that we
need the same infrastructure as a full progressbar anyway so we of
course have considered implementing this a few times but certainly we
will add some way to the API eventually and your input is very
welcome...:

> > 
> > The short answer is yes. Nico also briefly mentioned an idea on IRC to use
> > a mechanism similar to flashrom_set_log_callback() to avoid complicating
> > function signatures for the read, write, and verify functions.  
> 
> Correct, I would prefer a method to set the current callback and private
> pointer over adding more parameters. Though, we could have another set
> of functions ofc, with more parameters.
> 
> Richard, please let us know, what you'd prefer. After all, the interface
> should be designed to be most usable (and not driven by internals). e.g.
> do you expect the callback or the userdata pointer to change between
> calls?

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner