Re: RISC OS rescue mode

[Marco Gerards]

Timothy Baldwin <address@hidden> writes:

Hi Timothy,

Sorry for my late reaction. :(

Here are some comments to your email and to the new patch.

> On Tuesday 22 Nov 2005 09:47, Marco Gerards wrote:
>> Timothy Baldwin <address@hidden> writes:
>
>> > 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.
>
> How should that be done within the framework of our build system?

See Okuji's email, I hope it is useful.  Can you please fix it that
way?

>> > +# For grub_RO.img.
>
> Oops, wrong comment. The filename is "grub_RO,ff8".

You updated it in your tree?

>> > +grub_RO_ff8_SOURCES = kern/arm/aif_header.S \
>>
>> What's RO_ff8?
>
> The "_RO" is to distingush the RISC OS grub kernel, from, for example, a ARM 
> IEEE1275 grub kernel.  "ff8" is the RISC OS filetype for an executable file.
> I've changed this to lower case.

Thanks.

>>
>> > +#define SEEK_SET 0
>> > +#define SEEK_END 2
>>
>> can you use a prefix here?  for example GRUB_RISC_OS_SEEK_SET.
>
> Reasonable, however removing the prefix from the C library functions declared 
> in that file would make the code clearer for the reasons below, make code in 
> GRUB more portable and more consistent in style. We don't call open() 
> grub_posix_open()!

Right, but the POSIX functions are not used in the core.  And for the
userspace applications we just use the GLIBC headerfiles.  So I'd
prefer either a prefix or an include of system headerfiles.

> Perhaps we could use the library headers from gccsdk at 
> http://gccsdk.riscos.info/cgi-bin/cvsweb.cgi/gccsdk/libscl/ instead of 
> writing our own. When I started work on the RISC OS port, those headers 
> lacked a licence, now they have the BSD licence without the advertising 
> clause.  But we would need to declare the exports separately.

For GRUB I prefer that we have all copyrights for all files if
possible.  You already wrote your own headerfile so I prefer that.

>> > +#define Service_PreReset 0x45
>
>
>> Can you please change this so it is according to the GCS?
>> And please use a prefix.
>
> That would only serve to obfuscate the code:
>  - A prefix would suggest that GRUB implements these system calls.

No, the prefix is a namespace.  We did the same for IEEE 1275 calls.

>  - It would be harder to find the documention.

I think an experienced programmer does not have problems with that.

>  - Those are the standard system call names, and are universally used by RISC 
> OS programs, therefore the common ones easily recognisable by RISC OS 
> assembly language programmers. 

In that case, isn't it possible to include system headerfiles?

> And technically the names in this file is outside the scope of the GCS, by 
> virtue of only being included in RISC OS specific assembler.

I don't see why.  All sourcecode of GRUB is in the scope of the GCS.

>
>
>> Have you considered using the GRUB memory allocation routines?  What
>> are the advantages and disadvantages?
>
> Since the RISC OS C Library is used, it's memory allocation routines have 
> control of the memory in the application space. Not using the C library 
> results in a large amount of hard to debug assembly using infrequently used, 
> poorly documented, low-level interfaces.

That's a sane reason. :-)

>> And some general remarks:
>>
>> The copyright years are not always updated to 2005, is this correct?
>
> Some of it was written in 2004.

AFAIK the copyright years are for the year the patch enters CVS.  But
the GNU documentation regarding this issue is a bit vague, one could
say code is published when it is sent to the mailinglist.  But our
current practise is the year in which it will be committed.  So can
you change it to 2006?

>> 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.
>
> I hope I've corrected these, but I'm not sure what counts as a sentence.

Thanks!

I will just notice which you missed below.

> +typedef struct grub_risc_os_error
> +{
> +  unsigned number;
> +  char message[];
> +} *grub_risc_os_error_t;
> +
> +/* Number of last RISC OS error passed to grub_risc_os_error_convert */

You forgot a `.' here.

> +/* ISO C functions */

And here.

> +/* SharedCLibrary functions */

And here.

> +#define X(x) ((x) | 0x20000)

What is this?


> +lock:        b       lock    @ "Program exit instruction" (never reached)
> +     .word   grub_text_size
> +dsize:       .word   grub_data_size
> +     .word   0       @ Debug data size
> +zisize:      .word   grub_bss_size
> +     .word   0       @ No Debug data
> +     .word   _start
> +     .word   0       @ Workspace for self-moving image (not used)
> +     .word   32      @ Yes, we work in 32bit PC modes.
> +dstart:      .word   __data_start
> +     .word   0, 0    @ Unused
> +     nop             @ Initialise debugger (NOT!!)

Can you end the comments above with a `.'?

> +extern const char *grub_risc_os_kernel_command_string (void);

[...]

> +extern unsigned grub_arm_machine_serial_number[2];

Can these two be moved into a headerfile?


> --- grub2/kern/arm/risc_os/misc.c     1970-01-01 01:00:00.000000000 +0100
> +++ grub2_risc_os/kern/arm/risc_os/misc.c     2005-11-24 18:00:55.000000000 
> +0000
> @@ -0,0 +1,110 @@
> +/* misc.h - misc RISC OS bits */

This should be misc.c

Thanks,
Marco