[PATCH] fix disk->id abuse

[PATCH] fix disk->id abuse

[Robert Millan]

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


disk->id is supposed to be filled with a per-disk value so that the disk
cache manager can identify individual disks.

For single-disk drivers, a constant is enough.  But it seems someone started
using pointers to strings for them, and then we all (including me) copied.

Then I see disk/scsi.c doing the same thing, but this time it's a real problem
since it's not a single-disk driver.

So this patch means to solve both issues;  makes single-disk drivers use a
constant directly (since a pointer to string is meaningless and confusing),
and disk/scsi.c use LUNs which (I believe) will work as unique identifiers.

Marco, could you confirm this is ok (specially the latter)?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: disk_id_abuse.diff --]
[-- Type: text/x-diff, Size: 2116 bytes --]

2008-08-30  Robert Millan  <rmh@aybabtu.com>

	* disk/host.c (grub_host_open): Initialize `disk->id' using
	a constant rather than a pointer to string.
	* disk/memdisk.c (grub_memdisk_open): Likewise.
	* fs/i386/pc/pxe.c (grub_pxe_open): Likewise.
	* disk/scsi.c (grub_scsi_open): Initialize `disk->id' using the
	`lun' variable rather than a pointer to string.

Index: disk/scsi.c
===================================================================
--- disk/scsi.c	(revision 1831)
+++ disk/scsi.c	(working copy)
@@ -248,7 +248,7 @@ grub_scsi_open (const char *name, grub_d
     {
       if (! p->open (name, scsi))
 	{
-	  disk->id = (unsigned long) "scsi"; /* XXX */
+	  disk->id = lun;	/* FIXME: are LUNs unique identifiers? */
 	  disk->data = scsi;
 	  scsi->dev = p;
 	  scsi->lun = lun;
Index: disk/host.c
===================================================================
--- disk/host.c	(revision 1831)
+++ disk/host.c	(working copy)
@@ -41,7 +41,7 @@ grub_host_open (const char *name, grub_d
       return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a host disk");
 
   disk->total_sectors = 0;
-  disk->id = (unsigned long) "host";
+  disk->id = 'host';
   
   disk->has_partitions = 0;
   disk->data = 0;
Index: disk/memdisk.c
===================================================================
--- disk/memdisk.c	(revision 1831)
+++ disk/memdisk.c	(working copy)
@@ -41,7 +41,7 @@ grub_memdisk_open (const char *name, gru
       return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a memdisk");
 
   disk->total_sectors = memdisk_size / GRUB_DISK_SECTOR_SIZE;
-  disk->id = (unsigned long) "mdsk";
+  disk->id = 'mdsk';
   disk->has_partitions = 0;
 
   return GRUB_ERR_NONE;
Index: fs/i386/pc/pxe.c
===================================================================
--- fs/i386/pc/pxe.c	(revision 1831)
+++ fs/i386/pc/pxe.c	(working copy)
@@ -63,7 +63,7 @@ grub_pxe_open (const char *name, grub_di
       return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a pxe disk");
 
   disk->total_sectors = 0;
-  disk->id = (unsigned long) "pxe";
+  disk->id = 'pxe';
 
   disk->has_partitions = 0;
   disk->data = 0;

Re: [PATCH] fix disk->id abuse

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

> So this patch means to solve both issues;  makes single-disk drivers use a
> constant directly (since a pointer to string is meaningless and confusing),
> and disk/scsi.c use LUNs which (I believe) will work as unique identifiers.

Multi-character constants cause warnings.  That's why I changed them  
to strings.  Pointers to different strings are different, and that's  
all we need.  If you prefer numeric IDs, perhaps we'll need a header  
file with all IDs we support.  Alternatively, we need a macro that  
would convert characters to an ID.

-- 
Regards,
Pavel Roskin

Re: [PATCH] fix disk->id abuse

[Robert Millan]

On Sat, Aug 30, 2008 at 11:41:18AM -0400, Pavel Roskin wrote:
> Quoting Robert Millan <rmh@aybabtu.com>:
> 
> >So this patch means to solve both issues;  makes single-disk drivers use a
> >constant directly (since a pointer to string is meaningless and confusing),
> >and disk/scsi.c use LUNs which (I believe) will work as unique identifiers.
> 
> Multi-character constants cause warnings.

Can we silence them?

> That's why I changed them  
> to strings.  Pointers to different strings are different, and that's  
> all we need.

For single-disk drivers, yes.  But that has two problems:

  - People tend to think the string itself has a meaning.  We could avoid
    this by using "dummy" or so.

  - People tend to think it's fine to do the same for multi-disk drivers.
    We could avoid this by adding a short comment in each of them.

> If you prefer numeric IDs, perhaps we'll need a header  
> file with all IDs we support.  Alternatively, we need a macro that  
> would convert characters to an ID.

I agree that this is harder to maintain.  I'd be happy with any solution that
works correctly and prevents confusion.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] fix disk->id abuse

[Pavel Roskin]

On Sun, 2008-08-31 at 15:33 +0200, Robert Millan wrote:
> On Sat, Aug 30, 2008 at 11:41:18AM -0400, Pavel Roskin wrote:
> > Quoting Robert Millan <rmh@aybabtu.com>:
> > 
> > >So this patch means to solve both issues;  makes single-disk drivers use a
> > >constant directly (since a pointer to string is meaningless and confusing),
> > >and disk/scsi.c use LUNs which (I believe) will work as unique identifiers.
> > 
> > Multi-character constants cause warnings.
> 
> Can we silence them?

I don't think so.  Besides, strings are just as good as multi-character
constants.  In fact, strings are better, as they won't clash with any
pointers, simply because strings are pointers too, and they point to
areas in memory not used by other drivers.

> > That's why I changed them  
> > to strings.  Pointers to different strings are different, and that's  
> > all we need.
> 
> For single-disk drivers, yes.  But that has two problems:
> 
>   - People tend to think the string itself has a meaning.  We could avoid
>     this by using "dummy" or so.

There is a chance that pointers to "dummy" will be consolidated by the
linker.  I believe GNU ld won't do it, but it's not impossible.

>   - People tend to think it's fine to do the same for multi-disk drivers.
>     We could avoid this by adding a short comment in each of them.

Maybe we could rename "id" to something more descriptive.  But I don't
think it's a big concern.  Somebody writing a disk driver will need to
check the meaning of the disk structure.

We could write a macro for ID comparison that would compare both the
"driver ID" (disk->dev->id) and "device ID" (disk->id).  In this case,
we can omit disk->id initialization in the drivers supporting only one
device (e.g. memdisk) and only leave it where it's indeed needed for
identifying separate devices, thus removing potentially confusing code.

-- 
Regards,
Pavel Roskin

Re: [PATCH] fix disk->id abuse

[Robert Millan]

On Mon, Sep 01, 2008 at 07:39:29PM -0400, Pavel Roskin wrote:
> On Sun, 2008-08-31 at 15:33 +0200, Robert Millan wrote:
> > On Sat, Aug 30, 2008 at 11:41:18AM -0400, Pavel Roskin wrote:
> > > Quoting Robert Millan <rmh@aybabtu.com>:
> > > 
> > > >So this patch means to solve both issues;  makes single-disk drivers use a
> > > >constant directly (since a pointer to string is meaningless and confusing),
> > > >and disk/scsi.c use LUNs which (I believe) will work as unique identifiers.
> > > 
> > > Multi-character constants cause warnings.
> > 
> > Can we silence them?
> 
> I don't think so.  Besides, strings are just as good as multi-character
> constants.  In fact, strings are better, as they won't clash with any
> pointers, simply because strings are pointers too, and they point to
> areas in memory not used by other drivers.
> 
> > > That's why I changed them  
> > > to strings.  Pointers to different strings are different, and that's  
> > > all we need.
> > 
> > For single-disk drivers, yes.  But that has two problems:
> > 
> >   - People tend to think the string itself has a meaning.  We could avoid
> >     this by using "dummy" or so.
> 
> There is a chance that pointers to "dummy" will be consolidated by the
> linker.  I believe GNU ld won't do it, but it's not impossible.

Ok then.

> >   - People tend to think it's fine to do the same for multi-disk drivers.
> >     We could avoid this by adding a short comment in each of them.
> 
> Maybe we could rename "id" to something more descriptive.  But I don't
> think it's a big concern.  Somebody writing a disk driver will need to
> check the meaning of the disk structure.
> 
> We could write a macro for ID comparison that would compare both the
> "driver ID" (disk->dev->id) and "device ID" (disk->id).  In this case,
> we can omit disk->id initialization in the drivers supporting only one
> device (e.g. memdisk) and only leave it where it's indeed needed for
> identifying separate devices, thus removing potentially confusing code.

Sounds fine, although what worries me most if the current usage of 'id' in
scsi.c, which can lead to collision already.

I assume using LUNs is a proper solution for that one?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] fix disk->id abuse

[Marco Gerards]

[...]

>> We could write a macro for ID comparison that would compare both the
>> "driver ID" (disk->dev->id) and "device ID" (disk->id).  In this case,
>> we can omit disk->id initialization in the drivers supporting only one
>> device (e.g. memdisk) and only leave it where it's indeed needed for
>> identifying separate devices, thus removing potentially confusing code.
>
> Sounds fine, although what worries me most if the current usage of 'id' in
> scsi.c, which can lead to collision already.
>
> I assume using LUNs is a proper solution for that one?

No, it is not.  I think I already said so on IRC?

--
Marco

Re: [PATCH] fix disk->id abuse

[Christian Franke]

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

Marco Gerards wrote:
> [...]
>
>   
>>> We could write a macro for ID comparison that would compare both the
>>> "driver ID" (disk->dev->id) and "device ID" (disk->id).  In this case,
>>> we can omit disk->id initialization in the drivers supporting only one
>>> device (e.g. memdisk) and only leave it where it's indeed needed for
>>> identifying separate devices, thus removing potentially confusing code.
>>>       
>> Sounds fine, although what worries me most if the current usage of 'id' in
>> scsi.c, which can lead to collision already.
>>
>> I assume using LUNs is a proper solution for that one?
>>     
>
> No, it is not.  I think I already said so on IRC?
>
>   

If disk->id is supposed to be a GUID ('Grub Unique Identifier' in this 
case :-), then a pointer to the private data structure for the disk 
should work. This id is unique until disk close.

For drivers without disk->data, simply use the address of e.g. the open 
function itself.

See attached patch for an example.

Christian


[-- Attachment #2: grub2-disk-id.patch --]
[-- Type: text/x-diff, Size: 839 bytes --]

diff --git a/disk/memdisk.c b/disk/memdisk.c
index 978eae5..e814890 100644
--- a/disk/memdisk.c
+++ b/disk/memdisk.c
@@ -41,7 +41,7 @@ grub_memdisk_open (const char *name, grub_disk_t disk)
       return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a memdisk");
 
   disk->total_sectors = memdisk_size / GRUB_DISK_SECTOR_SIZE;
-  disk->id = (unsigned long) "mdsk";
+  disk->id = (unsigned long) grub_memdisk_open;
   disk->has_partitions = 0;
 
   return GRUB_ERR_NONE;
diff --git a/disk/scsi.c b/disk/scsi.c
index 01ef04e..8a5bd4e 100644
--- a/disk/scsi.c
+++ b/disk/scsi.c
@@ -248,7 +248,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)
     {
       if (! p->open (name, scsi))
 	{
-	  disk->id = (unsigned long) "scsi"; /* XXX */
+	  disk->id = (unsigned long) scsi;
 	  disk->data = scsi;
 	  scsi->dev = p;
 	  scsi->lun = lun;

Re: [PATCH] fix disk->id abuse

[Robert Millan]

On Tue, Sep 02, 2008 at 09:12:04PM +0200, Christian Franke wrote:
> 
> If disk->id is supposed to be a GUID ('Grub Unique Identifier' in this 
> case :-), then a pointer to the private data structure for the disk 
> should work. This id is unique until disk close.
> 
> For drivers without disk->data, simply use the address of e.g. the open 
> function itself.

This is fine for single-disk drivers, but for multi-disk ones we need it to
be unique among different disks provided by the same driver.

Although, of course, I don't see why can't we just make it use a pointer to
itself:

  disk->id = (unsigned long) &disk->id;

but then what's the point of storing that in a variable anyway.  We might as
well just remove this variable and whoever uses it can use a pointer to the
structure instead?

This works on the assumption that disk structures are never reallocated, but
I suppose that's a sane thing to assume...

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] fix disk->id abuse

[Christian Franke]

Robert Millan wrote:
> On Tue, Sep 02, 2008 at 09:12:04PM +0200, Christian Franke wrote:
>   
>> If disk->id is supposed to be a GUID ('Grub Unique Identifier' in this 
>> case :-), then a pointer to the private data structure for the disk 
>> should work. This id is unique until disk close.
>>
>> For drivers without disk->data, simply use the address of e.g. the open 
>> function itself.
>>     
>
> This is fine for single-disk drivers, but for multi-disk ones we need it to
> be unique among different disks provided by the same driver.
>
>   

I apparently made the false assumption, that multi-disk drivers always 
have disk->data as a real pointer.


> Although, of course, I don't see why can't we just make it use a pointer to
> itself:
>
>   disk->id = (unsigned long) &disk->id;
>
> but then what's the point of storing that in a variable anyway.  We might as
> well just remove this variable and whoever uses it can use a pointer to the
> structure instead?
>
> This works on the assumption that disk structures are never reallocated, but
> I suppose that's a sane thing to assume...
>
>   

Sounds good.

Probably add an inline function to improve readability:

typedef unsigned long grub_disk_id_t;

inline grub_disk_id_t disk_id (const grub_disk_t * disk)
{
  return (grub_id_t)(disk);
}

...
data = grub_disk_cache_fetch (disk->dev->id, disk_id(disk), start_sector);


Christian

Re: [PATCH] fix disk->id abuse

[Robert Millan]

On Tue, Sep 02, 2008 at 03:55:18PM +0200, Marco Gerards wrote:
> 
> [...]
> 
> >> We could write a macro for ID comparison that would compare both the
> >> "driver ID" (disk->dev->id) and "device ID" (disk->id).  In this case,
> >> we can omit disk->id initialization in the drivers supporting only one
> >> device (e.g. memdisk) and only leave it where it's indeed needed for
> >> identifying separate devices, thus removing potentially confusing code.
> >
> > Sounds fine, although what worries me most if the current usage of 'id' in
> > scsi.c, which can lead to collision already.
> >
> > I assume using LUNs is a proper solution for that one?
> 
> No, it is not.  I think I already said so on IRC?

If you did, I must have missed it.  In any case, what can we use instead of
that?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."