/kern/file.c BUG

/kern/file.c BUG

[Oleg Strikov]

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

Incorrect behavior of grub_file_open () function in e.g. loop context:

char *file_names[] =
{
"(hd0,1)/file1", //file do not exist
"(hd0,1)/file2"  //file exist
};
grub_file_t file;
int i;
for (i = 0; i < 2; i++)
{
     file  = grub_file_open (file_names[i]);
     if (file) {...}
}

There, we should get positive return in the second case (i == 1), but
grub_file_open() returns 0.

Using gdb i've found that this problem connected with incorrect errno check
in /kern/file.c

Let's look:

>grub_file_t
>grub_file_open (const char *name)
>{
>  grub_device_t device;
>  grub_file_t file = 0;
>  char *device_name;
>  char *file_name;

>  device_name = grub_file_get_device_name (name);
>  if (grub_errno)
>    return 0;

But, we DO NOT set grub_errno to 0 at the begining of the function, thats
why next loop round it always returns 0



PATCH:
--- ./grub2.stable/kern/file.c.orig    2008-01-23 22:30:17.850755634 +0000
+++ ./grub2.stable/kern/file.c    2008-01-23 22:45:17.788904935 +0000
@@ -59,6 +59,8 @@ grub_file_open (const char *name)
   char *device_name;
   char *file_name;

+  grub_errno = 0;
+
   device_name = grub_file_get_device_name (name);
   if (grub_errno)
     return 0;

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

Re: /kern/file.c BUG

[Robert Millan]

On Wed, Jan 23, 2008 at 11:00:57PM +0000, Oleg Strikov wrote:
> Incorrect behavior of grub_file_open () function in e.g. loop context:
> 
> char *file_names[] =
> {
> "(hd0,1)/file1", //file do not exist
> "(hd0,1)/file2"  //file exist
> };
> grub_file_t file;
> int i;
> for (i = 0; i < 2; i++)
> {
>      file  = grub_file_open (file_names[i]);
>      if (file) {...}
> }
> 
> There, we should get positive return in the second case (i == 1), but
> grub_file_open() returns 0.
> 
> Using gdb i've found that this problem connected with incorrect errno check
> in /kern/file.c
> 
> Let's look:
> 
> >grub_file_t
> >grub_file_open (const char *name)
> >{
> >  grub_device_t device;
> >  grub_file_t file = 0;
> >  char *device_name;
> >  char *file_name;
> 
> >  device_name = grub_file_get_device_name (name);
> >  if (grub_errno)
> >    return 0;
> 
> But, we DO NOT set grub_errno to 0 at the begining of the function, thats
> why next loop round it always returns 0

Fixed, thank you.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: /kern/file.c BUG

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Wed, Jan 23, 2008 at 11:00:57PM +0000, Oleg Strikov wrote:
>> Incorrect behavior of grub_file_open () function in e.g. loop context:
>>
>> char *file_names[] =
>> {
>> "(hd0,1)/file1", //file do not exist
>> "(hd0,1)/file2"  //file exist
>> };
>> grub_file_t file;
>> int i;
>> for (i = 0; i < 2; i++)
>> {
>>      file  = grub_file_open (file_names[i]);
>>      if (file) {...}
>> }
>>
>> There, we should get positive return in the second case (i == 1), but
>> grub_file_open() returns 0.
>>
>> Using gdb i've found that this problem connected with incorrect errno check
>> in /kern/file.c
>>
>> Let's look:
>>
>>> grub_file_t
>>> grub_file_open (const char *name)
>>> {
>>>  grub_device_t device;
>>>  grub_file_t file = 0;
>>>  char *device_name;
>>>  char *file_name;
>>>  device_name = grub_file_get_device_name (name);
>>>  if (grub_errno)
>>>    return 0;
>> But, we DO NOT set grub_errno to 0 at the begining of the function, thats
>> why next loop round it always returns 0
> 
> Fixed, thank you.

Now you broke it ;)

Previous behavior was working correctly. You have to handle errorcodes 
at some point and that means when error is handled it is zeroed (or 
GRUB_ERR_NONE). So code is in callee where that loop was.

Re: /kern/file.c BUG

[Pavel Roskin]

On Thu, 2008-01-24 at 20:08 +0200, Vesa Jääskeläinen wrote:

> Previous behavior was working correctly. You have to handle
> errorcodes 
> at some point and that means when error is handled it is zeroed (or 
> GRUB_ERR_NONE). So code is in callee where that loop was.

I suggest that we never set grub_errno to 0 (except the initialization).
That would match the standard errno behavior:

http://www.opengroup.org/onlinepubs/009695399/functions/errno.html

-- 
Regards,
Pavel Roskin

Re: /kern/file.c BUG

[Yoshinori K. Okuji]

On Thursday 24 January 2008 19:34, Pavel Roskin wrote:
> On Thu, 2008-01-24 at 20:08 +0200, Vesa Jääskeläinen wrote:
> > Previous behavior was working correctly. You have to handle
> > errorcodes
> > at some point and that means when error is handled it is zeroed (or
> > GRUB_ERR_NONE). So code is in callee where that loop was.
>
> I suggest that we never set grub_errno to 0 (except the initialization).
> That would match the standard errno behavior:
>
> http://www.opengroup.org/onlinepubs/009695399/functions/errno.html

Marco is right. As you pointed out, our error handling is different from errno 
on Unix, but this is intentional, because I stole the model from GRUB Legacy 
and Parted.

Okuji

Re: /kern/file.c BUG

[Oleg Strikov]

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

On Jan 24, 2008 9:42 PM, Oleg Strikov <oleg.strikov@gmail.com> wrote:

>
> >> Previous behavior was working correctly. You have to handle
> > >> errorcodes
> > >> at some point and that means when error is handled it is zeroed (or
> > >> GRUB_ERR_NONE). So code is in callee where that loop was.
> >
> > >I suggest that we never set grub_errno to 0 (except the
> > initialization).
> > >That would match the standard errno behavior:
> >
> > >http://www.opengroup.org/onlinepubs/009695399/functions/errno.html
> > <http://www.opengroup.org/onlinepubs/009695399/functions/errno.html>
> >
> > >--
> > >Regards,
> > >Pavel Roskin
>
>
> But is it correct to check and handle  errno in some `library` function
> (now we do) ?  I CAN, but i do not have to examine  errno after each
> non-error-free call; is it right?
>
>

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

Re: /kern/file.c BUG

[Marco Gerards]

"Yoshinori K. Okuji" <okuji@enbug.org> writes:

> On Thursday 24 January 2008 19:34, Pavel Roskin wrote:
>> On Thu, 2008-01-24 at 20:08 +0200, Vesa Jääskeläinen wrote:
>> > Previous behavior was working correctly. You have to handle
>> > errorcodes
>> > at some point and that means when error is handled it is zeroed (or
>> > GRUB_ERR_NONE). So code is in callee where that loop was.
>>
>> I suggest that we never set grub_errno to 0 (except the initialization).
>> That would match the standard errno behavior:
>>
>> http://www.opengroup.org/onlinepubs/009695399/functions/errno.html
>
> Marco is right. As you pointed out, our error handling is different from errno 
> on Unix, but this is intentional, because I stole the model from GRUB Legacy 
> and Parted.

It's nice when people say I am right (it should happen more often).
But I do not recall participating in this thread.  Did I miss
something, like I usually do? ;-)

--
Marco

Re: /kern/file.c BUG

[Robert Millan]

On Thu, Jan 24, 2008 at 11:09:17PM +0100, Marco Gerards wrote:
> 
> It's nice when people say I am right (it should happen more often).

You're right.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: /kern/file.c BUG

[Robert Millan]

On Thu, Jan 24, 2008 at 10:19:55PM +0100, Yoshinori K. Okuji wrote:
> On Thursday 24 January 2008 19:34, Pavel Roskin wrote:
> > On Thu, 2008-01-24 at 20:08 +0200, Vesa Jääskeläinen wrote:
> > > Previous behavior was working correctly. You have to handle
> > > errorcodes
> > > at some point and that means when error is handled it is zeroed (or
> > > GRUB_ERR_NONE). So code is in callee where that loop was.
> >
> > I suggest that we never set grub_errno to 0 (except the initialization).
> > That would match the standard errno behavior:
> >
> > http://www.opengroup.org/onlinepubs/009695399/functions/errno.html
> 
> Marco is right. As you pointed out, our error handling is different from errno 
> on Unix, but this is intentional, because I stole the model from GRUB Legacy 
> and Parted.

Could you explain how is it supposed to work?  There were clearly two bugs,
which I "fixed" in grub_disk_open first, and in grub_file_open.  The solution
can be wrong, but the bugs still existed.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: /kern/file.c BUG

[Pavel Roskin]

On Thu, 2008-01-24 at 21:43 +0000, Oleg Strikov wrote:

>         But is it correct to check and handle  errno in some `library`
>         function (now we do) ?  I CAN, but i do not have to examine
>         errno after each non-error-free call; is it right? 

I don't know how grub_errno is supposed to work, so I cannot comment on
that.

But normally it's OK to read errno if it's known that some error has
happened.  It makes it possible to have one error handler that would not
specify which exactly call has failed.  For instance, it's not important
if an I/O error happened when opening the file or when reading from it,
or when closing it.

It should be OK to set errno to 0 only if it's definitely known that the
callers (not necessarily the immediate caller) don't expect errno to be
preserved.  Therefore, setting errno to 0 in a library would be wrong.

-- 
Regards,
Pavel Roskin

Re: /kern/file.c BUG

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Thu, 2008-01-24 at 20:08 +0200, Vesa Jääskeläinen wrote:
>
>> Previous behavior was working correctly. You have to handle
>> errorcodes 
>> at some point and that means when error is handled it is zeroed (or 
>> GRUB_ERR_NONE). So code is in callee where that loop was.
>
> I suggest that we never set grub_errno to 0 (except the initialization).
> That would match the standard errno behavior:
>
> http://www.opengroup.org/onlinepubs/009695399/functions/errno.html

That just wouldn't work, it is not how the current code was designed.

For example when you open a file, first the filesystem type has to be
detected.  So first you try filesystem A, after that filesystem B,
etc.  A returns an error, B too, etc.  Some of these errors (out of
memory) should immediately be reported back to the caller.  Others,
like wrong filesystem type are normal.  We just clear the error and
continue.

It's like exception handling in C++.  You can choose which errors you
handle at a certain level.

--
Marco

Re: /kern/file.c BUG

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Thu, Jan 24, 2008 at 10:19:55PM +0100, Yoshinori K. Okuji wrote:
>> On Thursday 24 January 2008 19:34, Pavel Roskin wrote:
>> > On Thu, 2008-01-24 at 20:08 +0200, Vesa Jääskeläinen wrote:
>> > > Previous behavior was working correctly. You have to handle
>> > > errorcodes
>> > > at some point and that means when error is handled it is zeroed (or
>> > > GRUB_ERR_NONE). So code is in callee where that loop was.
>> >
>> > I suggest that we never set grub_errno to 0 (except the initialization).
>> > That would match the standard errno behavior:
>> >
>> > http://www.opengroup.org/onlinepubs/009695399/functions/errno.html
>> 
>> Marco is right. As you pointed out, our error handling is different from errno 
>> on Unix, but this is intentional, because I stole the model from GRUB Legacy 
>> and Parted.
>
> Could you explain how is it supposed to work?  There were clearly two bugs,
> which I "fixed" in grub_disk_open first, and in grub_file_open.  The solution
> can be wrong, but the bugs still existed.

Hopefully my previous mail makes that clear?

Were these previous problems committed?

What is the bug?

--
Marco

Re: /kern/file.c BUG

[Marco Gerards]

Vesa Jääskeläinen <chaac@nic.fi> writes:

(replying to this mail, I didn't receive the first mail over the list...)

> Robert Millan wrote:
>> On Wed, Jan 23, 2008 at 11:00:57PM +0000, Oleg Strikov wrote:
>>> Incorrect behavior of grub_file_open () function in e.g. loop context:
>>>
>>> char *file_names[] =
>>> {
>>> "(hd0,1)/file1", //file do not exist
>>> "(hd0,1)/file2"  //file exist
>>> };
>>> grub_file_t file;
>>> int i;
>>> for (i = 0; i < 2; i++)
>>> {
>>>      file  = grub_file_open (file_names[i]);
>>>      if (file) {...}
>>> }
>>>
>>> There, we should get positive return in the second case (i == 1), but
>>> grub_file_open() returns 0.

No, that is not right.  This behavior is correct.

When you open the first file, you get an error which you ignore.  If
you think this error is harmless, clear it.  So your code is wrong.

--
Marco

Re: /kern/file.c BUG

[Robert Millan]

On Fri, Jan 25, 2008 at 09:50:14AM +0100, Marco Gerards wrote:
> Vesa Jääskeläinen <chaac@nic.fi> writes:
> 
> (replying to this mail, I didn't receive the first mail over the list...)
> 
> > Robert Millan wrote:
> >> On Wed, Jan 23, 2008 at 11:00:57PM +0000, Oleg Strikov wrote:
> >>> Incorrect behavior of grub_file_open () function in e.g. loop context:
> >>>
> >>> char *file_names[] =
> >>> {
> >>> "(hd0,1)/file1", //file do not exist
> >>> "(hd0,1)/file2"  //file exist
> >>> };
> >>> grub_file_t file;
> >>> int i;
> >>> for (i = 0; i < 2; i++)
> >>> {
> >>>      file  = grub_file_open (file_names[i]);
> >>>      if (file) {...}
> >>> }
> >>>
> >>> There, we should get positive return in the second case (i == 1), but
> >>> grub_file_open() returns 0.
> 
> No, that is not right.  This behavior is correct.
> 
> When you open the first file, you get an error which you ignore.  If
> you think this error is harmless, clear it.  So your code is wrong.

Ok, I reverted it.  This will uncover another bug somewhere else.  I can't
remember what it was about, though.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: /kern/file.c BUG

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Fri, Jan 25, 2008 at 09:50:14AM +0100, Marco Gerards wrote:
>> Vesa Jääskeläinen <chaac@nic.fi> writes:
>> 
>> (replying to this mail, I didn't receive the first mail over the list...)
>> 
>> > Robert Millan wrote:
>> >> On Wed, Jan 23, 2008 at 11:00:57PM +0000, Oleg Strikov wrote:
>> >>> Incorrect behavior of grub_file_open () function in e.g. loop context:
>> >>>
>> >>> char *file_names[] =
>> >>> {
>> >>> "(hd0,1)/file1", //file do not exist
>> >>> "(hd0,1)/file2"  //file exist
>> >>> };
>> >>> grub_file_t file;
>> >>> int i;
>> >>> for (i = 0; i < 2; i++)
>> >>> {
>> >>>      file  = grub_file_open (file_names[i]);
>> >>>      if (file) {...}
>> >>> }
>> >>>
>> >>> There, we should get positive return in the second case (i == 1), but
>> >>> grub_file_open() returns 0.
>> 
>> No, that is not right.  This behavior is correct.
>> 
>> When you open the first file, you get an error which you ignore.  If
>> you think this error is harmless, clear it.  So your code is wrong.
>
> Ok, I reverted it.  This will uncover another bug somewhere else.  I can't
> remember what it was about, though.

Thanks.  Can you look that up?

If you found it, I can help you fixing it, if you'd like?

--
Marco

Re: /kern/file.c BUG

[Robert Millan]

On Sat, Jan 26, 2008 at 01:04:04PM +0100, Marco Gerards wrote:
> >
> > Ok, I reverted it.  This will uncover another bug somewhere else.  I can't
> > remember what it was about, though.
> 
> Thanks.  Can you look that up?

Sorry, I can't.  I don't remember what I found broken, but chances are
somebody will run through it again.

> If you found it, I can help you fixing it, if you'd like?

It wasn't a difficult bug.  The problem is just I don't know which bug is it!

Don't worry, we'll eventually run through it.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)