fix disk->id abuse

PATCH In review Robert Millan 4 participants 10 messages Aug 30, 2008 – Sep 2, 2008

lore

Share & tools

lore

Robert Millan

author

2008-08-30 12:28 UTC #0 lore raw

PATCH 1/1 fix disk->id abuse

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)?

Signature · 5 lines

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."

2008-08-30 Robert Millan <[email protected]>

* 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.

scsi.c +1 -1

Index: disk/scsi.c
===================================================================
@@ -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;

host.c +1 -1

Index: disk/host.c
===================================================================
@@ -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;

memdisk.c +1 -1

Index: disk/memdisk.c
===================================================================
@@ -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;

i386/pc/pxe.c +1 -1

Index: fs/i386/pc/pxe.c
===================================================================
@@ -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;

Pavel Roskin

2008-08-30 15:41 UTC #1 lore raw

Quoting Robert Millan <[email protected]>:

Quoted 3 lines

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.

Signature · 2 lines

Regards,
Pavel Roskin

Robert Millan

author

2008-08-31 13:35 UTC #2 lore raw

Quoted 7 lines · Pavel Roskin

Quoting Robert Millan <[email protected]>:
Quoted 3 lines
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?

Quoted 3 lines

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.

Quoted 3 lines

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.

Signature · 5 lines

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."

Pavel Roskin

2008-09-01 23:39 UTC #3 lore raw

Quoted 10 lines · Robert Millan

Quoted 7 lines · Pavel Roskin
Quoting Robert Millan <[email protected]>:
Quoted 3 lines
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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 2 lines

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

Signature · 2 lines

Regards,
Pavel Roskin

Robert Millan

author

2008-09-02 13:42 UTC #4 lore raw

Quoted 21 lines · Pavel Roskin

Quoted 10 lines · Robert Millan
Quoted 7 lines · Pavel Roskin
Quoting Robert Millan <[email protected]>:
Quoted 3 lines
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.
Quoted 4 lines
Quoted 3 lines
That's why I changed them
to strings. Pointers to different strings are different, and that's all we need.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 7 lines

Quoted 2 lines
- 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.

Quoted 5 lines

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?

Signature · 5 lines

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."

Marco Gerards

2008-09-02 13:49 UTC #5 lore raw

[...]

Quoted 10 lines

Quoted 5 lines
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

Christian Franke

2008-09-02 19:12 UTC #6 lore raw

Quoted 9 lines · Marco Gerards

[...]
Quoted 6 lines
Quoted 6 lines
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.

Quoted 9 lines

Quoted 5 lines
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

disk/memdisk.c +1 -1

index 978eae5..e814890 100644
@@ -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;

disk/scsi.c +1 -1

index 01ef04e..8a5bd4e 100644
@@ -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;

Robert Millan

author

2008-09-02 19:26 UTC #7 lore raw

Quoted 7 lines · Christian Franke

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

Signature · 5 lines

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."

Christian Franke

2008-09-02 19:43 UTC #8 lore raw

Quoted 11 lines · Robert Millan

Quoted 4 lines · Christian Franke
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.

Quoted 13 lines

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

Robert Millan

author

2008-09-02 19:22 UTC #9 lore raw

In reply to Marco Gerards

Quoted 9 lines · Marco Gerards

[...]
Quoted 6 lines
Quoted 5 lines
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.

Quoted 6 lines

Quoted 4 lines
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?

Signature · 5 lines

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."