Re: [flashrom] [PATCH] Kill globals, initialize programmer-related variables explicitly

[Michael Karcher]

Am Freitag, den 02.07.2010, 16:13 +0200 schrieb Carl-Daniel Hailfinger:
> > > /* ichspi.c */
> > >  extern int ichspi_lock;
> > >  extern uint32_t ichspi_bbar;
> > > +extern void *ich_spibar;
> >
> > Do we really need this global, if...

[... code from board_enable.c ...]
> > ...we move all this stuff in a new, non-static function in ichspi.c?
>
> That means we get two new global symbols (enable_flash_vt8237s_spi() and
> enable_flash_ich_dc_spi()) to make one global symbol (ich_spibar) go
> away. It may look appealing at first, but in the end there is a net loss.

It might be a matter of my taste, but I consider global variable much
more evil than global functions. But we shouldn't spend energy on
discussing this - just do as you like.
> > use shutdown_fn_count as loop variable, instead of an extra i, like
> > while (shutdown_fn_count > 0) {
> >     int j = --shutdown_fn_count;
> >     shutdown_fn[j].func(shutdown_fn[j].data);
> > }
> >
> > This makes sure that even if shutdown_fn[j] calls exit and we atexit()
> > our generic shutdown function, no function will be called twice.
>
> Nice, thanks.

No problem. That's what the review is for :)
> > > +/* programmer_param is programmer-specific, but it MUST NOT be initialized in
> > > programmer_init() because it is initialized in the command line parser.
> > > */
> > >
> > > char *programmer_param = NULL;
> >
> > So why not make this a parameter of programmer_init() ?
>
> Existing code sometimes changes the location where programmer_param
> points to. That means we'd have to pass &programmer_param to various
> init functions instead of just passing programmer_param. I have a patch
> which cleans up programmer_param usage, and that patch will make such a
> change easier.

OK, so we keep this as-is now. I still think that we can change even now
how the variable is initialized. Current state is that the variable is
initalized from the command line parser (which is client code). Even if
it stays a global variable, I think we should have the client pass the
pointer through programmer_init into the library than having it directly
access global variables, but again, we don't have to beat this horse to
death. I just want to make the point clear.
> Index: flashrom-explicit_init/pcidev.c
>
> --- flashrom-explicit_init/pcidev.c	(Revision 1066)
> +++ flashrom-explicit_init/pcidev.c	(Arbeitskopie)
> @@ -25,11 +25,11 @@
>
> uint32_t io_base_addr;
>  struct pci_access *pacc;
> -struct pci_filter filter;
> +static struct pci_filter filter;

It seems like the filter is only used in pcidev_init. Can't we make it a
local stack variable in that function?

Anyway, this is tentatively
Acked-by: Michael Karcher

Please either explain why "filter" must be global or make it local, use
the Ack in either case.

Regards,
  Michael Karcher