Re: [flashrom] [PATCH] register_shutdown for execution on programmer shutdown

[Carl-Daniel Hailfinger]

On 13.02.2010 00:59, Michael Karcher wrote:
> Am Freitag, den 12.02.2010, 03:59 +0100 schrieb Carl-Daniel Hailfinger:
> > +#define SHUTDOWN_MAXFN 4
> > +static int shutdown_fn_count = 0;
> > +void (*shutdown_fn[SHUTDOWN_MAXFN]) (void);
>
> style hint: Use a typedef for the function pointer type:
> typedef void (*shutdown_proc)(void);

I thought about using a typedef, but given that most people expect
typedefs in the form
typedef type identifier;
whereas the suggested typedef has the form
typedef first-part-of-type identifier second-part-of-type;
a typedef may actually decrease readability for people to don't know
that such typedefs exist.
> shutdown_proc shutdownfn[SHUTDOWN_MAXFN];

About the name of the typedef: I think we should use the name
atexit_func or something like that to make it obvious we're basically
copying atexit.
> > +int register_shutdown(void (*function)(void))
>
> And this gets "int register_shutdown(shutdown_proc function)

While we're discussing typedefs, I'm not 100% sure we really want to
make this an atexit clone with all limitations. For example, some people
may want to undo any mainboard GPIO settings on shutdown, and for this,
allowing a void * parameter to functions registered with
register_shutdown would allow such functions to access a struct/whatever
with GPIO undo information.

However, if we use a struct, the typedef issue won't have a noticeable
impact anymore AFAICS.

struct shutdown_function_data {
    void (*func) (void *),
    void *data
} shutdown_fn[4];
int register_shutdown(void (*func)(void *), void *data);

What do you think?
> Except for the style hint, everything looks great, so
> Acked-by: Michael Karcher

Thanks for the review. I'll wait with a commit until you comment on my
review response.

Regards,
Carl-Daniel