Re: Ethernet support [PATCH]

[Johan Rydberg]

Marco Gerards <address@hidden> writes:

> If I do not get any comments within a week, I'll just commit the
> patch.  > 

I have a few more comments;

> +#include <grub/types.h>
> +#include <grub/err.h>
> +
> +enum grub_memstack_type
> +  {
> +    GRUB_MEMSTACK_TYPE_ETHERNET,
> +    GRUB_MEMSTACK_TYPE_IPV4,
> +    GRUB_MEMSTACK_TYPE_ARP,
> +    GRUB_MEMSTACK_TYPE_UDP,
> +    GRUB_MEMSTACK_TYPE_DATA /* XXX */
> +  };
> +typedef enum grub_memstack_type grub_memstack_type_t;

I do not see why this is needed at all?  The data should be opaque.

> +grub_memstack_t grub_memstack_new (void);
> +
> +void grub_memstack_free (grub_memstack_t stack);
> +
> +void *grub_memstack_push (grub_memstack_t stack,
> +                       grub_memstack_type_t type,
> +                       grub_size_t size);
> +
> +void *grub_memstack_bottom (grub_memstack_t stack,
> +                         grub_memstack_type_t type,
> +                         grub_size_t size);
> +
> +void *grub_memstack_pop (grub_memstack_t stack, grub_size_t *size);
> +
> +grub_err_t grub_memstack_popall (grub_memstack_t stack,
> +                              char *buf, grub_size_t *size);
> +

The network component in a operating system is often seen as a stack,
because packets travel up and down through a logical stack of
functionality layers.  That doesn't say that the data that is passed
is really a stack.  BSD uses a data structure called "mbufs", which
really just is a linked list of small data chunks.  There are
operations to append, prepend and gather data from the "mbuf".

I suggest to remove the word "stack" from our data structure, and
replace it with something more generic; "buf" maybe?  

And since it is really a linked list, _push should become prepend, and
_bottom append, respectively.  

Also, I do not think that the read operation should be destructive.
Maybe a _read function is in order?

If we do this, we have a shot at adding a data structure that could be
used in other places than just our small network stack.


~j

pgpkko6I9LlfB.pgp
Description: PGP signature