Re: RISC OS rescue mode

[Marco Gerards]

Timothy Baldwin <address@hidden> writes:

Hi Timothy,

Thanks a lot for your patch.  This week I will have very little time,
but I'll try to review it ASAP.  Here are a few things I noticed while
I was having a quick look at it.

>> I'm a little confused about this "RISC_OS" naming. Is "RISC_OS" the name of
>> the firmware that GRUB runs above? If not, I don't think that's an
>> appropriate name.
>
> The firmware is called "RISC OS", not to be confused with "RISC/os". Putting 
> spaces in the filenames is asking for trouble.
>
> RISC OS is a co-operative multitasking operating system and is the firmware 
> in 
> the systems on which it runs.

Please use `risc_os' instead of `RISC_OS'.  I don't like using capital
names in files, we don't do that for other files.  For example for
IEEE 1275 we use ieee1275.  Please only use capital names in case of
macro's and enum's, see the GCS.

> 2005-08-29  Timothy Baldwin <address@hidden>
>         * boot/arm/RISC_OS/!Run,feb: New file.

I would prefer another name and renaming when installing.  I wonder
what other people think about that.

>         * Makefile.in (RMKFILES): Add arm-RISC_OS.rmk and common.rmk to list.

Wasn't common.rmk added already?

> +# For grub_RO.img.
> +grub_RO_ff8_SOURCES = kern/arm/aif_header.S \

What's RO_ff8?

> +#define SEEK_SET 0
> +#define SEEK_END 2

can you use a prefix here?  for example GRUB_RISC_OS_SEEK_SET.

> +extern unsigned grub_RISC_OS_version, grub_RISC_OS_81C710_present;

Please only put one variable declaration on  a line.

> +#define Cache_Control 0x280
[...]
> +#define Service_PreReset 0x45
> +#define ERROR_NO_SUCH_SWI 0x1E6

Can you please change this so it is according to the GCS?  And please
use a prefix.  Or did yo directly copy it from some other file?  In
that case we need to figure out the copyright issues.

> +void *
> +grub_memalign (grub_size_t align, grub_size_t size)
> +{
> +  void **mem = grub_RISC_OS_malloc (size + align + sizeof (void *));
> +  if (mem == 0)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      return 0;
> +    }
> +  void **result = (void **) grub_align (align, (unsigned) (mem + 1));
> +  result[-1] = mem;
> +  return result;
> +}
> +
> +void *
> +grub_malloc (grub_size_t size)
> +{
> +  void **result = grub_RISC_OS_malloc (size + sizeof (void *));
> +  if (result == 0)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      return 0;
> +    }
> +  *result = result;
> +  return result + 1;
> +}
> +
> +void *
> +grub_realloc (void *ptr, grub_size_t size)
> +{
> +  void **ptr2 = ptr;
> +  void **result =
> +    grub_RISC_OS_realloc (ptr2 ? ptr2[-1] : 0, size + sizeof (void *));
> +  if (result == 0)
> +    {
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      return 0;
> +    }
> +  *result = result;
> +  return result + 1;
> +}

Have you considered using the GRUB memory allocation routines?  What
are the advantages and disadvantages?


And some general remarks:

The copyright years are not always updated to 2005, is this correct?
Sometimes you mentioned copyright with your name.  Can that be
changed?

Not all C comments are formatted correctly.  Always start a sentence with a
capital letter and end with a `.'.  Put two spaces after a sentence.

A full review will follow.

Thanks,
Marco