Re: gcs doubt #1

Re: gcs doubt #1

[Marco Gerards]

adrian15 <adrian15@raulete.net> writes:

> Please put a comment on each function saying what the function does,
> what sorts of arguments it gets, and what the possible values of
> arguments mean and are used for.
>
> Is it ok that the search.c and test.c (commands/ folder) do not have any
> of these comments at all?

The currect test.c is a dummy so the `if' expression can be
used/tested.

If you can provide a patch for sourcecode to improve or add comments
in a sane way, I am willing to commit those patches.  Just please
don't make the mistake to add useless comments.

--
Marco

Re: test -e patch

[adrian15]

> adrian15 <adrian15@raulete.net> writes:
> 
>> > Attached you will find the patch adding test -e support for grub2.
>> >
>> > This is my first patch. I have compiled it without no errors.
> 
> Urgh... I thought/hoped I told you I had a test.c rewrite sitting on
> my harddisk?  Or did I tell Robert to poke me until next weekend so I
> will work on it?  It includes everything you expect from test.c,
> expect the cleanup and testing.  ;-) 

Do you mean you also have the '-e' option ?


> Please have a look at the wiki.  It has quite some information about
> GRUB 2.
Whenever possible I'll download some info from the wiki.

>> > Should I write "Test if a file exists" instead of "test if a file
>> > exists" or "FILE exists"?
> 
> FILE

FILE
or
FILE exists ?

Or have you coded it yourself too?

>> > How the hell should I treat grub errors. Maybe the test_file_exists has
>> > to return a static grub_err_t and then from grub_cmd_test I should call
>> > it like this: return (test_file_exists (args[0])) so that the error
>> > propagates ?
> 
> Right.  Otherwise the error will be lost.

Ok. I will try that every function propagates errors.

> 
>> > What are the error polices ?
> 
> Returning the err_t that grub_error returns when possible.
Ok.
>> > One question. Should we put the test-e function into a separate file or
>> > not ?
> 
> No, the problem is that the design of test.c (which is just a
> placeholder) is wrong.  It needs a proper parser for the arguments and
> a way to deal with this...

Ok. We will wait for your code.

>> > The question is if the user will see the -e, -f or other options when
>> > querying the test command help or not ?
> 
> They should.  But I am not sure if the final version will support
> this.  Especially because of the nested syntax of the test arguments.

Do you mean the -e options support
or
do you mean the -e options showing at help test support ?

>> > +static void
>> > +test_file_exists (const char *key)
> 
> Why not filename?

test_filename_exists
or
filename

?

>> >  {
>> > +
> 
> You accidently introduced a whiteline.

No whitelines after an initial {. I write down it.

>> > +  if (state[0].set)
>> > +    test_file_exists (args[0]);
>> > +  else
>> > +  {
> 
> This means that this check is run for any other expression.  This is
> quite error sensitive.

In my code the only implemented option is '-e'. When there will be more
I could add more nested if with the other options, or maybe better we
will enjoy your improved code.


adrian15

Re: gcs doubt #1

[adrian15]

>> > Please put a comment on each function saying what the function does,
>> > what sorts of arguments it gets, and what the possible values of
>> > arguments mean and are used for.
>> >
>> > Is it ok that the search.c and test.c (commands/ folder) do not have any
>> > of these comments at all?
> 
> The currect test.c is a dummy so the `if' expression can be
> used/tested.
> 
> If you can provide a patch for sourcecode to improve or add comments
> in a sane way, I am willing to commit those patches.  Just please
> don't make the mistake to add useless comments.

An useful comment is one that:
	-Describes a function algorithm
	-Describes what a function does (as a black box)
	-Boot of them ?

adrian15

Re: GSoC: CD-ROM booting options

[adrian15]

> Do we want to be able to boot the computer from GRUB2 installed on a
> medium (whatever it may be), then be able to boot from a CD  (in
> whatever mode - emulation or no emulation)?
> 
> Do we also want to be able to boot the computer through the BIOS from
> a CD on which GRUB2 was installed and continue booting from the CD
> (like GRUB legacy does)?
> 
> Do we want to support both?
> 
> Personally, I think both are important to support, but I want to see
> what the majority of developers and users think we should support.

	Although a lot of people want you to begin with the second option and
although it's a good thing for an hipothetical super grub2 disk... I
want to remind you what's the okuji thought about this:

=======
>> My name is Alex Roman and I have been selected as a Summer of Code
>> > > student for the GRUB2 project. I will be adding CD-ROM booting
>> > > functionality to GRUB2.
> >
> > Welcome! Cdrom booting funcionality for grub2 is a good piece of news.
> >
>> > > The first stage of the project will attempt to use BIOS calls
>> > > to achieve this functionality (which is, of course, dependent on the
>> > > BIOS's capabilities, standards compliance, etc.).
> >
> > Ok. That's how it works in Grub legacy I suppose.

Nope. This is a different thing. This is a new feature which is not
present in GRUB Legacy.

Although people often mistake this, booting a CD from GRUB is quite
different from booting a GRUB installed into a CD. GRUB Legacy has the
latter feature, but not the former one.

GRUB 2 needs to support both cases, but I prefer to see something not
present in GRUB Legacy implemented in GRUB 2 first.

=======

But do as you want, maybe it is better to begin with the easiest task
and then doing the more difficult task.

adrian15

Re: gcs doubt #1

[Marco Gerards]

adrian15 <adrian15@raulete.net> writes:

>>> > Please put a comment on each function saying what the function does,
>>> > what sorts of arguments it gets, and what the possible values of
>>> > arguments mean and are used for.
>>> >
>>> > Is it ok that the search.c and test.c (commands/ folder) do not have any
>>> > of these comments at all?
>>
>> The currect test.c is a dummy so the `if' expression can be
>> used/tested.
>>
>> If you can provide a patch for sourcecode to improve or add comments
>> in a sane way, I am willing to commit those patches.  Just please
>> don't make the mistake to add useless comments.
>
> An useful comment is one that:
> 	-Describes a function algorithm
> 	-Describes what a function does (as a black box)
> 	-Boot of them ?

Yes, but I oppose to adding obvious comments.  But patches will be
reviewed, so you will learn fast enough what we want :-).

--
Marco

Re: test -e patch

[Marco Gerards]

adrian15 <adrian15@raulete.net> writes:

>> adrian15 <adrian15@raulete.net> writes:
>>
>>> > Attached you will find the patch adding test -e support for grub2.
>>> >
>>> > This is my first patch. I have compiled it without no errors.
>>
>> Urgh... I thought/hoped I told you I had a test.c rewrite sitting on
>> my harddisk?  Or did I tell Robert to poke me until next weekend so I
>> will work on it?  It includes everything you expect from test.c,
>> expect the cleanup and testing.  ;-)
>
> Do you mean you also have the '-e' option ?

I mean I have everything that is possible.  That includes `-e', but
also all other features.

Something noone looked at is "expr".

>> Please have a look at the wiki.  It has quite some information about
>> GRUB 2.
> Whenever possible I'll download some info from the wiki.
>
>>> > Should I write "Test if a file exists" instead of "test if a file
>>> > exists" or "FILE exists"?
>>
>> FILE
>
> FILE
> or
> FILE exists ?

FILE exists

> Or have you coded it yourself too?

Hm?

[...]

>> No, the problem is that the design of test.c (which is just a
>> placeholder) is wrong.  It needs a proper parser for the arguments and
>> a way to deal with this...
>
> Ok. We will wait for your code.

:-)

Sorry for the confusion :-/

>>> > The question is if the user will see the -e, -f or other options when
>>> > querying the test command help or not ?
>>
>> They should.  But I am not sure if the final version will support
>> this.  Especially because of the nested syntax of the test arguments.
>
> Do you mean the -e options support
> or
> do you mean the -e options showing at help test support ?

Well, the version for GNU/Linux doesn't show help text.  Perhaps it
cannot be implemented using the argument parser in a clean way.  I do
not remember.

>>> > +static void
>>> > +test_file_exists (const char *key)
>>
>> Why not filename?
>
> test_filename_exists
> or
> filename

I mean instead of key.

> ?
>
>>> >  {
>>> > +
>>
>> You accidently introduced a whiteline.
>
> No whitelines after an initial {. I write down it.

Well, usually adding whitespaces around code you didn't change is
wrong or dirty.

>>> > +  if (state[0].set)
>>> > +    test_file_exists (args[0]);
>>> > +  else
>>> > +  {
>>
>> This means that this check is run for any other expression.  This is
>> quite error sensitive.
>
> In my code the only implemented option is '-e'. When there will be more
> I could add more nested if with the other options, or maybe better we
> will enjoy your improved code.

Sure.  Just poke me a lot during the weekend on IRC ;-)

--
Marco

gcs doubt #1

[adrian15]

In the GNU Coding Standards they say in:

5.2 Commenting Your Work

Please put a comment on each function saying what the function does,
what sorts of arguments it gets, and what the possible values of
arguments mean and are used for.


Is it ok that the search.c and test.c (commands/ folder) do not have any
of these comments at all?

You do not comment them at all?
You comment them in another place?
Maybe it's ok with the help description that it is set when you register
a command?
Maybe this is a bug and should be fixed?

adrian15

Re: gcs doubt #1

[adrian15]

>>> If you can provide a patch for sourcecode to improve or add comments
>>> in a sane way, I am willing to commit those patches.  Just please
>>> don't make the mistake to add useless comments.
>> An useful comment is one that:
>> 	-Describes a function algorithm
>> 	-Describes what a function does (as a black box)
>> 	-Boot of them ?
> 
> Yes, but I oppose to adding obvious comments.  But patches will be
> reviewed, so you will learn fast enough what we want :-).

You had to choose one of the three options :).
Well... it doesn't matter. I've understood that I have to send patches
and learn by trial-error.

adrian15

Re: GSoC: CD-ROM booting options

[Alex Roman]

On 05/06/07, adrian15 <adrian15@raulete.net> wrote:
> GRUB 2 needs to support both cases, but I prefer to see something not
> present in GRUB Legacy implemented in GRUB 2 first.

Good point... In any case there's some commonality between the two
cases and for now I'm working to see if I can read some ElTorito data
structures from a bootable CD... Which brings me to the question:

Is it possible to call int 13h functions from a GRUB2 module? If yes,
what is the (preferred?) way to do it?


Thanks in advance!

-- 
Alex Roman <alex.roman@gmail.com>

Re: GSoC: CD-ROM booting options

[Vesa Jääskeläinen]

Alex Roman wrote:
> On 05/06/07, adrian15 <adrian15@raulete.net> wrote:
>> GRUB 2 needs to support both cases, but I prefer to see something not
>> present in GRUB Legacy implemented in GRUB 2 first.
> 
> Good point... In any case there's some commonality between the two
> cases and for now I'm working to see if I can read some ElTorito data
> structures from a bootable CD... Which brings me to the question:
> 
> Is it possible to call int 13h functions from a GRUB2 module? If yes,
> what is the (preferred?) way to do it?

You need to add new kernel thunk(s) to grub2/kern/i386/pc/startup.S and
then make them jump to 16b real mode and then return back to protected
mode. There are lots of examples in that file. Then make proper include
file for your calls (most likely you can add it to existing one in this
include file grub2/include/grub/i386/pc/biosdisk.h).

I would suggest to make testing command that accesses those kernel
thunks. This way it is easier to test out those API calls.

Re: GSoC: CD-ROM booting options

[Alex Roman]

That's what I plan on doing... I want to write a module that for now
just allows me to test various int 13h calls, look at their return
values and get a general idea of how things work.

Thanks for the idea Vesa!


Cheers!

On 07/06/07, Vesa Jääskeläinen <chaac@nic.fi> wrote:
> Alex Roman wrote:
> > On 05/06/07, adrian15 <adrian15@raulete.net> wrote:
> >> GRUB 2 needs to support both cases, but I prefer to see something not
> >> present in GRUB Legacy implemented in GRUB 2 first.
> >
> > Good point... In any case there's some commonality between the two
> > cases and for now I'm working to see if I can read some ElTorito data
> > structures from a bootable CD... Which brings me to the question:
> >
> > Is it possible to call int 13h functions from a GRUB2 module? If yes,
> > what is the (preferred?) way to do it?
>
> You need to add new kernel thunk(s) to grub2/kern/i386/pc/startup.S and
> then make them jump to 16b real mode and then return back to protected
> mode. There are lots of examples in that file. Then make proper include
> file for your calls (most likely you can add it to existing one in this
> include file grub2/include/grub/i386/pc/biosdisk.h).
>
> I would suggest to make testing command that accesses those kernel
> thunks. This way it is easier to test out those API calls.
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Alex Roman <alex.roman@gmail.com>