[PATCH] Don't abort if prefix is not set

[PATCH] Don't abort if prefix is not set

[Pavel Roskin]

---

 ChangeLog |    5 +++++
 kern/dl.c |    6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 009b4dc..44d5887 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2008-01-26  Pavel Roskin  <proski@gnu.org>
+
+	* kern/dl.c (grub_dl_load): Don't abort if prefix is not set,
+	just return an error.
+
 2008-01-26  Bean  <bean123ch@gmail.com>
 
 	* fs/reiserfs.c (grub_fshelp_node): New member next_offset.
diff --git a/kern/dl.c b/kern/dl.c
index d3488fb..9e8c24a 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -625,8 +625,10 @@ grub_dl_load (const char *name)
   if (mod)
     return mod;
   
-  if (! grub_dl_dir)
-    grub_fatal ("module dir is not initialized yet");
+  if (! grub_dl_dir) {
+    grub_error (GRUB_ERR_FILE_NOT_FOUND, "\"prefix\" is not set");
+    return 0;
+  }
 
   filename = (char *) grub_malloc (grub_strlen (grub_dl_dir) + 1
 				   + grub_strlen (name) + 4 + 1);

Re: [PATCH] Don't abort if prefix is not set

[Robert Millan]

On Sat, Jan 26, 2008 at 02:44:44PM -0500, Pavel Roskin wrote:
> 
> ---
> 
>  ChangeLog |    5 +++++
>  kern/dl.c |    6 ++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 009b4dc..44d5887 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2008-01-26  Pavel Roskin  <proski@gnu.org>
> +
> +	* kern/dl.c (grub_dl_load): Don't abort if prefix is not set,
> +	just return an error.
> +
>  2008-01-26  Bean  <bean123ch@gmail.com>
>  
>  	* fs/reiserfs.c (grub_fshelp_node): New member next_offset.
> diff --git a/kern/dl.c b/kern/dl.c
> index d3488fb..9e8c24a 100644
> --- a/kern/dl.c
> +++ b/kern/dl.c
> @@ -625,8 +625,10 @@ grub_dl_load (const char *name)
>    if (mod)
>      return mod;
>    
> -  if (! grub_dl_dir)
> -    grub_fatal ("module dir is not initialized yet");
> +  if (! grub_dl_dir) {
> +    grub_error (GRUB_ERR_FILE_NOT_FOUND, "\"prefix\" is not set");
> +    return 0;

Seems fine, but are you sure this error is handled somewhere?

-- 
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: [PATCH] Don't abort if prefix is not set

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

>> +  if (! grub_dl_dir) {
>> +    grub_error (GRUB_ERR_FILE_NOT_FOUND, "\"prefix\" is not set");
>> +    return 0;
>
> Seems fine, but are you sure this error is handled somewhere?

If prefix is not set, the module is not loaded.  What other handling  
do we need?

I found that prefix would not be set on PowerPC if /memory/available  
is missing.  Even though I have a better workaround for it now, I  
still think that aborting for such minor reason is wrong.  After all,  
the user may unset prefix.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Don't abort if prefix is not set

[Robert Millan]

On Sat, Jan 26, 2008 at 03:11:02PM -0500, Pavel Roskin wrote:
> Quoting Robert Millan <rmh@aybabtu.com>:
> 
> >>+  if (! grub_dl_dir) {
> >>+    grub_error (GRUB_ERR_FILE_NOT_FOUND, "\"prefix\" is not set");
> >>+    return 0;
> >
> >Seems fine, but are you sure this error is handled somewhere?
> 
> If prefix is not set, the module is not loaded.  What other handling  
> do we need?

What I mean is, is the error message printed?  I recall it had to be handled
somewhere.  Or maybe I'm confused...

> I found that prefix would not be set on PowerPC if /memory/available  
> is missing.

Sounds strange.. how is that so?

> Even though I have a better workaround for it now, I  
> still think that aborting for such minor reason is wrong.  After all,  
> the user may unset prefix.

Agreed..

-- 
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: [PATCH] Don't abort if prefix is not set

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

> On Sat, Jan 26, 2008 at 03:11:02PM -0500, Pavel Roskin wrote:
>> Quoting Robert Millan <rmh@aybabtu.com>:
>>
>> >>+  if (! grub_dl_dir) {
>> >>+    grub_error (GRUB_ERR_FILE_NOT_FOUND, "\"prefix\" is not set");
>> >>+    return 0;
>> >
>> >Seems fine, but are you sure this error is handled somewhere?
>>
>> If prefix is not set, the module is not loaded.  What other handling
>> do we need?
>
> What I mean is, is the error message printed?  I recall it had to be handled
> somewhere.  Or maybe I'm confused...

It's printed.  Tested with grub-emu and in qemu.

>> I found that prefix would not be set on PowerPC if /memory/available
>> is missing.
>
> Sounds strange.. how is that so?

Because the environment is allocated in the heap, I believe.

>> Even though I have a better workaround for it now, I
>> still think that aborting for such minor reason is wrong.  After all,
>> the user may unset prefix.
>
> Agreed..

Applying.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Don't abort if prefix is not set

[Vesa Jääskeläinen]

Robert Millan wrote:
> On Sat, Jan 26, 2008 at 03:11:02PM -0500, Pavel Roskin wrote:
>> Quoting Robert Millan <rmh@aybabtu.com>:
>>
>>>> +  if (! grub_dl_dir) {
>>>> +    grub_error (GRUB_ERR_FILE_NOT_FOUND, "\"prefix\" is not set");
>>>> +    return 0;
>>> Seems fine, but are you sure this error is handled somewhere?
>> If prefix is not set, the module is not loaded.  What other handling  
>> do we need?
> 
> What I mean is, is the error message printed?  I recall it had to be handled
> somewhere.  Or maybe I'm confused...

Whole error stack is printed just before prompt.

Re: [PATCH] Don't abort if prefix is not set

[Robert Millan]

On Sat, Jan 26, 2008 at 03:31:34PM -0500, Pavel Roskin wrote:
> >>I found that prefix would not be set on PowerPC if /memory/available
> >>is missing.
> >
> >Sounds strange.. how is that so?
> 
> Because the environment is allocated in the heap, I believe.

It is, but I'd have expected GRUB to issue grub_fatal() when this happens. :-/

-- 
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 /.)