[PATCH] Split #2: parser/reader separation

[PATCH] Split #2: parser/reader separation

[Bean]

[-- Attachment #1: Type: text/plain, Size: 251 bytes --]

Hi,

This patch moves the parser from normal.mod to script/sh, it also
split rescue mode to rescue_reader and rescue_parser.

dynamic command support and auto fs loader is moved to standalone
source file normal/dyncmd.c and normal/autofs.c.

-- 
Bean

[-- Attachment #2: s2.zip --]
[-- Type: application/zip, Size: 30324 bytes --]

Re: [PATCH] Split #2: parser/reader separation

[Bean]

Hi,

ping ?

On Sun, Apr 12, 2009 at 12:36 AM, Bean <bean123ch@gmail.com> wrote:
> Hi,
>
> This patch moves the parser from normal.mod to script/sh, it also
> split rescue mode to rescue_reader and rescue_parser.
>
> dynamic command support and auto fs loader is moved to standalone
> source file normal/dyncmd.c and normal/autofs.c.
>
> --
> Bean
>



-- 
Bean

Re: [PATCH] Split #2: parser/reader separation

[Vladimir Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

Hello. Could you say how it influences the size of kernel and also write a
changlog? I looked at it and it seems to be ok but I'm not sure that I
haven't missed few files between moved functions. I haven't tested the patch
yet

On Wed, Apr 22, 2009 at 7:47 AM, Bean <bean123ch@gmail.com> wrote:

> Hi,
>
> ping ?
>
> On Sun, Apr 12, 2009 at 12:36 AM, Bean <bean123ch@gmail.com> wrote:
> > Hi,
> >
> > This patch moves the parser from normal.mod to script/sh, it also
> > split rescue mode to rescue_reader and rescue_parser.
> >
> > dynamic command support and auto fs loader is moved to standalone
> > source file normal/dyncmd.c and normal/autofs.c.
> >
> > --
> > Bean
> >
>
>
>
> --
> Bean
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 1479 bytes --]

Re: [PATCH] Split #2: parser/reader separation

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

Hello this patch breaks  grub-emu. Also some files include grub/rescue.h and
this has to be fixed.
Also there is a bug is that in case of syntax error reader continues to ask
for more lines. E.g
grub> if
>;
syntax error
>
Then it's impossible to exit from this "bug mode". It's not your fault but
since you touch this code could you fix this?
This patch increases the size of kernel by 504 bytes and the size of
core.img by 224 bytes. Is there a way to do the same thing in a more compact
way?
Also it would be good to write the current parser in "grub>" prompt (not
necessary for rescue prompt)

On Sat, Apr 11, 2009 at 6:36 PM, Bean <bean123ch@gmail.com> wrote:

> Hi,
>
> This patch moves the parser from normal.mod to script/sh, it also
> split rescue mode to rescue_reader and rescue_parser.
>
> dynamic command support and auto fs loader is moved to standalone
> source file normal/dyncmd.c and normal/autofs.c.
>
> --
> Bean
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 1722 bytes --]

Re: [PATCH] Split #2: parser/reader separation

[Bean]

On Sun, Apr 26, 2009 at 11:50 PM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> Hello this patch breaks  grub-emu. Also some files include grub/rescue.h and
> this has to be fixed.

Hi,

For large patch, I normally add only the necessary changes to make it
compile in i386-pc. As commits are quite frequent, this would minimize
the effort to sync with svn head. I'd fix those small issue before
final commitment.

> Also there is a bug is that in case of syntax error reader continues to ask
> for more lines. E.g
> grub> if
>>;
> syntax error
>>
> Then it's impossible to exit from this "bug mode". It's not your fault but
> since you touch this code could you fix this?

I believe a proper way to solve this is to add an eol character, for
example ctrl-D. Then we have a way to break from console input. But
this fix is not trivial, perhaps it should be in a separate patch.

> This patch increases the size of kernel by 504 bytes and the size of
> core.img by 224 bytes. Is there a way to do the same thing in a more compact
> way?

Oh, I think it is quite compact already, have you got any suggestion
to reduce its size ?

> Also it would be good to write the current parser in "grub>" prompt (not
> necessary for rescue prompt)

Good point.

-- 
Bean

Re: [PATCH] Split #2: parser/reader separation

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

On Mon, Apr 27, 2009 at 7:51 AM, Bean <bean123ch@gmail.com> wrote:

> On Sun, Apr 26, 2009 at 11:50 PM, Vladimir 'phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> > Hello this patch breaks  grub-emu. Also some files include grub/rescue.h
> and
> > this has to be fixed.
>
> Hi,
>
> For large patch, I normally add only the necessary changes to make it
> compile in i386-pc. As commits are quite frequent, this would minimize
> the effort to sync with svn head. I'd fix those small issue before
> final commitment.
>
Ok. However a Changlog would simplify the review because then it would be
easy to see which parts are newly written and which is just code moving
around

>
> > Also there is a bug is that in case of syntax error reader continues to
> ask
> > for more lines. E.g
> > grub> if
> >>;
> > syntax error
> >>
> > Then it's impossible to exit from this "bug mode". It's not your fault
> but
> > since you touch this code could you fix this?
>
> I believe a proper way to solve this is to add an eol character, for
> example ctrl-D. Then we have a way to break from console input. But
> this fix is not trivial, perhaps it should be in a separate patch.
>
Ok

>
> > This patch increases the size of kernel by 504 bytes and the size of
> > core.img by 224 bytes. Is there a way to do the same thing in a more
> compact
> > way?
>
> Oh, I think it is quite compact already, have you got any suggestion
> to reduce its size ?
>
I'll have a look at it again today or tomorrow

>
> > Also it would be good to write the current parser in "grub>" prompt (not
> > necessary for rescue prompt)
>
> Good point.
>
> --
> Bean
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 3212 bytes --]

Re: [PATCH] Split #2: parser/reader separation

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

Patch looks fine for me. I think you can commit it
As for lua put the origin clearly and put the original copyright notice
below grub's one. Tomorrow I'll do the same for freebsd64 patch and will
merge it

> Oh, I think it is quite compact already, have you got any suggestion
>> to reduce its size ?
>>
> The only possibility I've seen is to move grub_parser_execute out of
kernel. However I don't  think it's practicable.

>
>
>> > Also it would be good to write the current parser in "grub>" prompt (not
>> > necessary for rescue prompt)
>>
>> Good point.
>>
>> --
>> Bean
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 1969 bytes --]

Re: [PATCH] Split #2: parser/reader separation

[Bean]

Hi,

Thanks for the review, commit it now.

On Sat, May 2, 2009 at 6:15 AM, Vladimir 'phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> Patch looks fine for me. I think you can commit it
> As for lua put the origin clearly and put the original copyright notice
> below grub's one. Tomorrow I'll do the same for freebsd64 patch and will
> merge it
>>>
>>> Oh, I think it is quite compact already, have you got any suggestion
>>> to reduce its size ?
>
> The only possibility I've seen is to move grub_parser_execute out of kernel.
> However I don't  think it's practicable.
>>
>>>
>>> > Also it would be good to write the current parser in "grub>" prompt
>>> > (not
>>> > necessary for rescue prompt)
>>>
>>> Good point.
>>>
>>> --
>>> Bean
>>>
>>>
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/grub-devel
>>
>>
>>
>> --
>> Regards
>> Vladimir 'phcoder' Serbinenko
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Bean