[PATCH] Add host open devicename check

[PATCH] Add host open devicename check

[Christian Franke]

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

grub-emu ls command does not list any partitions.

Device scan in grub_disk_open() stops early because grub_host_open() 
returns success on all device names.

Christian

2007-10-25  Christian Franke  <franke@computer.org>

	* disk/host.c (grub_host_open): Add check for "host". This fixes
	the problem that grub-emu does not find partitions.



[-- Attachment #2: grub2-host_open.patch --]
[-- Type: text/x-patch, Size: 503 bytes --]

--- grub2.orig/disk/host.c	2007-08-02 19:24:05.000000000 +0200
+++ grub2/disk/host.c	2007-10-13 15:11:18.000000000 +0200
@@ -34,8 +34,11 @@ grub_host_iterate (int (*hook) (const ch
 }
 
 static grub_err_t
-grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
+grub_host_open (const char *name, grub_disk_t disk)
 {
+  if (grub_strcmp(name, "host"))
+      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
+
   disk->total_sectors = 0;
   disk->id = (int) "host";
   

Re: [PATCH] Add host open devicename check

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> grub-emu ls command does not list any partitions.
>
> Device scan in grub_disk_open() stops early because grub_host_open()
> returns success on all device names.
>
> Christian
>
> 2007-10-25  Christian Franke  <franke@computer.org>
>
> 	* disk/host.c (grub_host_open): Add check for "host". This fixes
> 	the problem that grub-emu does not find partitions.

Please mention the attribute change.

> --- grub2.orig/disk/host.c	2007-08-02 19:24:05.000000000 +0200
> +++ grub2/disk/host.c	2007-10-13 15:11:18.000000000 +0200
> @@ -34,8 +34,11 @@ grub_host_iterate (int (*hook) (const ch
>  }
>  
>  static grub_err_t
> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
> +grub_host_open (const char *name, grub_disk_t disk)
>  {
> +  if (grub_strcmp(name, "host"))
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");

Please add a space after the function name and before the "(".

--
Marco

Re: [PATCH] Add host open devicename check

[Robert Millan]

On Thu, Oct 25, 2007 at 09:51:38PM +0200, Christian Franke wrote:
>  static grub_err_t
> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
> +grub_host_open (const char *name, grub_disk_t disk)
>  {
> +  if (grub_strcmp(name, "host"))
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
> +

I would test for (disk->id == GRUB_DISK_DEVICE_HOST_ID) instead.  Faster,
and also cleaner/simpler IMHO.

-- 
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] Add host open devicename check

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Thu, Oct 25, 2007 at 09:51:38PM +0200, Christian Franke wrote:
>>  static grub_err_t
>> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
>> +grub_host_open (const char *name, grub_disk_t disk)
>>  {
>> +  if (grub_strcmp(name, "host"))
>> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
>> +
>
> I would test for (disk->id == GRUB_DISK_DEVICE_HOST_ID) instead.  Faster,
> and also cleaner/simpler IMHO.

It's not possible unfortunately :-(.  This information is about to be
filled in in this same function.  Besides that, you have grub_disk_dev
in mind.

--
Marco

Re: [PATCH] Add host open devicename check

[Robert Millan]

On Fri, Nov 09, 2007 at 10:17:19PM +0100, Marco Gerards wrote:
> Robert Millan <rmh@aybabtu.com> writes:
> 
> > On Thu, Oct 25, 2007 at 09:51:38PM +0200, Christian Franke wrote:
> >>  static grub_err_t
> >> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
> >> +grub_host_open (const char *name, grub_disk_t disk)
> >>  {
> >> +  if (grub_strcmp(name, "host"))
> >> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
> >> +
> >
> > I would test for (disk->id == GRUB_DISK_DEVICE_HOST_ID) instead.  Faster,
> > and also cleaner/simpler IMHO.
> 
> It's not possible unfortunately :-(.  This information is about to be
> filled in in this same function.

Still seems like an ugly hack to me.  Oh well :-/

> Besides that, you have grub_disk_dev
> in mind.

Ouch ;-)

-- 
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] Add host open devicename check

[Christian Franke]

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

Marco Gerards wrote:
>> ..
>>
>> 2007-10-25  Christian Franke  <franke@computer.org>
>>
>> 	* disk/host.c (grub_host_open): Add check for "host". This fixes
>> 	the problem that grub-emu does not find partitions.
>>     
>
> Please mention the attribute change.
>
>   
>> ...
>>  static grub_err_t
>> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
>> +grub_host_open (const char *name, grub_disk_t disk)
>>  {
>> +  if (grub_strcmp(name, "host"))
>> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
>>     
>
> Please add a space after the function name and before the "(".
>
>   

Done

Christian

2007-11-10  Christian Franke  <franke@computer.org>

	* disk/host.c (grub_host_open): Remove attribute unused from
	name parameter. Add check for "host". This fixes the problem
	that grub-emu does not find partitions.



[-- Attachment #2: grub2-host_open-2.patch --]
[-- Type: text/x-patch, Size: 504 bytes --]

--- grub2.orig/disk/host.c	2007-08-02 19:24:05.000000000 +0200
+++ grub2/disk/host.c	2007-11-10 14:18:11.046875000 +0100
@@ -34,8 +34,11 @@ grub_host_iterate (int (*hook) (const ch
 }
 
 static grub_err_t
-grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
+grub_host_open (const char *name, grub_disk_t disk)
 {
+  if (grub_strcmp (name, "host"))
+      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
+
   disk->total_sectors = 0;
   disk->id = (int) "host";
   

Re: [PATCH] Add host open devicename check

[Christian Franke]

Robert Millan wrote:
> On Fri, Nov 09, 2007 at 10:17:19PM +0100, Marco Gerards wrote:
>   
>> Robert Millan <rmh@aybabtu.com> writes:
>>
>>     
>>> On Thu, Oct 25, 2007 at 09:51:38PM +0200, Christian Franke wrote:
>>>       
>>>>  static grub_err_t
>>>> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
>>>> +grub_host_open (const char *name, grub_disk_t disk)
>>>>  {
>>>> +  if (grub_strcmp(name, "host"))
>>>> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
>>>> +
>>>>         
>>> I would test for (disk->id == GRUB_DISK_DEVICE_HOST_ID) instead.  Faster,
>>> and also cleaner/simpler IMHO.
>>>       
>> It's not possible unfortunately :-(.  This information is about to be
>> filled in in this same function.
>>     
>
> Still seems like an ugly hack to me.  Oh well :-/
>
>   

All disk/* modules' open routines check whether the name (hd%d, ata%d, 
...) is valid and return UNKNOWN_DEVICE on error.
The missing name check in host.c is a bug which can IMO only be fixed 
this way.

Christian

Re: [PATCH] Add host open devicename check

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> Marco Gerards wrote:
>>> ..
>>>
>>> 2007-10-25  Christian Franke  <franke@computer.org>
>>>
>>> 	* disk/host.c (grub_host_open): Add check for "host". This fixes
>>> 	the problem that grub-emu does not find partitions.
>>>
>>
>> Please mention the attribute change.
>>
>>
>>> ...
>>>  static grub_err_t
>>> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
>>> +grub_host_open (const char *name, grub_disk_t disk)
>>>  {
>>> +  if (grub_strcmp(name, "host"))
>>> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
>>>
>>
>> Please add a space after the function name and before the "(".
>>
>>
>
> Done
>
> Christian
>
> 2007-11-10  Christian Franke  <franke@computer.org>
>
> 	* disk/host.c (grub_host_open): Remove attribute unused from
> 	name parameter. Add check for "host". This fixes the problem
> 	that grub-emu does not find partitions.

Looks fine to me.

--
Marco

Re: [PATCH] Add host open devicename check

[Marco Gerards]

Robert Millan <rmh@aybabtu.com> writes:

> On Fri, Nov 09, 2007 at 10:17:19PM +0100, Marco Gerards wrote:
>> Robert Millan <rmh@aybabtu.com> writes:
>> 
>> > On Thu, Oct 25, 2007 at 09:51:38PM +0200, Christian Franke wrote:
>> >>  static grub_err_t
>> >> -grub_host_open (const char *name __attribute((unused)), grub_disk_t disk)
>> >> +grub_host_open (const char *name, grub_disk_t disk)
>> >>  {
>> >> +  if (grub_strcmp(name, "host"))
>> >> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
>> >> +
>> >
>> > I would test for (disk->id == GRUB_DISK_DEVICE_HOST_ID) instead.  Faster,
>> > and also cleaner/simpler IMHO.
>> 
>> It's not possible unfortunately :-(.  This information is about to be
>> filled in in this same function.
>
> Still seems like an ugly hack to me.  Oh well :-/

It isn't.  The driver gets a string that it can use to determine if it
has control over this disk or not.  `grub_disk_t disk' is there to be
filled in if it has, it isn't initialised yet.

--
Marco

Re: [PATCH] Add host open devicename check

[Robert Millan]

On Sat, Nov 10, 2007 at 05:04:25PM +0100, Marco Gerards wrote:
> >
> > 2007-11-10  Christian Franke  <franke@computer.org>
> >
> > 	* disk/host.c (grub_host_open): Remove attribute unused from
> > 	name parameter. Add check for "host". This fixes the problem
> > 	that grub-emu does not find partitions.
> 
> Looks fine to me.

Committed.

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