Re: Handlers

[Marco Gerards <mgerards@xs4all.nl>]

"Neal H. Walfield" <neal@walfield.org> writes:

> I know know why you call this a handler; it seems to me that it is
> just a semi-generic list package.  Am I missing something?

Perhaps.  I can better explain how this can be used and give a small
example.  GRUB has several types of handlers.  I hope a handler is the
right word, please correct me if it's not.  Examples of handlers are
filesystems, terminals, partitioning schemes, commands, etc.  A
handler usually consists of a struct with function pointers and a
pointer to the next handler of its kind.

Let's focus on filesystems.  To implement the filesystem handler, we
defined 3 basic functions.  You need to be able to register and
unregister filesystems.  It should be possible to iterate over all
filesystems.  grub_file_open, for example, iterates over all
filesystems to see if it detects a certain filesystem layout on some
disk.

What we currently did is rewriting grub_*_register, grub_*_unregister
and grub_*_iterate.  It's boring to rewrite such functions all the
time and this results in duplicated code.

So it is not a list in the classical sense, it does not contain data.

> You can find a slighly more flexible and generic implementation here:
>
>   http://cvs.savannah.gnu.org/viewvc/hurd-l4/viengoos/list.h?root=hurd&view=markup
>
> I've been using that for a while and am quite satisfied with it.
>
> Perhaps you'll find it useful.

It certainly looks good, thanks for the suggestion.  However, I do not
think we have the same goals.  For example, I focus on size and do not
need many features.

> Two comments:
>
> At Fri, 29 Aug 2008 14:36:56 +0200,
> Marco Gerards wrote:
>> +void
>> +grub_handler_unregister (grub_handler_t *head, grub_handler_t handler)
>> +{
>> +  grub_handler_t *p, q;
>> +
>> +  for (p = head, q = *p; q; p = &(q->next), q = q->next)
>                     ^^^^^^                     ^^^^^^^^^^^
>
> This is a bit inconsistent.
>
>   for (p = head, q = *head; q; p = &(q->next), q = q->next)
>
> or
>
>   for (p = head, q = *p; q; p = &(q->next), q = *p)
>
>
> Or, more succinctly:
>
>   for (p = head; (q = *p); p = &(q->next))

You are right, I simply blindly copied this from existing code.  I can
make this more consistent.

>> +int
>> +grub_handler_iterate (grub_handler_t head,
>> +		      int (*hook) (const grub_handler_t handler))
>> +{
>> +  grub_handler_t p;
>> +
>> +  for (p = head; p; p = p->next)
>> +    if (hook (p))
>> +      break;
>> +
>> +  return 0;
>> +}
>
> A suggestion: when HOOK returns a non-zero value, return that from the
> function:
>
>      for (p = head; p; p = p->next)
>        {
>          int ret = hook (p);
>          if (ret)
>            return ret;
>        }
>      return 0;

Well spotted!  Normally we use:

if (hook (p))
  return 1;

This is what happens when you blindly copy existing code, it might not
meet the behavior you expect :-/

--
Marco