Strange code in ide_cdrom_register

Strange code in ide_cdrom_register

[Peter Chubb]

Hi,
	This code snippet in ide_cdrom_register() seems really
strange...

	devinfo->ops = &ide_cdrom_dops;
	devinfo->mask = 0;
>>>	*(int *)&devinfo->speed = CDROM_STATE_FLAGS (drive)->current_speed;
>>>	*(int *)&devinfo->capacity = nslots;
	devinfo->handle = (void *) drive;
	strcpy(devinfo->name, drive->name);

devinfo->speed and devinfo->capacity are both ints.  So the casts are
just a disaster waiting to happen, if the types of capacity or speed
ever change?

Peter C
--
You are lost in a maze of bitkeeper repositories, all slightly different.

Re: Strange code in ide_cdrom_register

[Michael Dunsky]

Hi!

Peter Chubb wrote:
 > Hi,
 > 	This code snippet in ide_cdrom_register() seems really
 > strange...
 >
 > 	devinfo->ops = &ide_cdrom_dops;
 > 	devinfo->mask = 0;
 >
 >>>>	*(int *)&devinfo->speed = CDROM_STATE_FLAGS (drive)->current_speed;
 >>>>	*(int *)&devinfo->capacity = nslots;
 >>>
 > 	devinfo->handle = (void *) drive;
 > 	strcpy(devinfo->name, drive->name);
 >
 > devinfo->speed and devinfo->capacity are both ints.  So the casts are
 > just a disaster waiting to happen, if the types of capacity or speed
 > ever change?

Just take a quick look in drivers/ide/ide-cd.h: values "nslots" and
"current_speed" are of type "byte", so we need to cast to store them 
(like that) into the integer-vars. Nothing strange there....


 > Peter C
 >

ciao

Michael

Re: Strange code in ide_cdrom_register

[Peter Chubb]

>>>>> "Michael" == Michael Dunsky <michael.dunsky@p4all.de> writes:

Michael> Hi!  Peter Chubb wrote:
PeterC> Hi, This code snippet in ide_cdrom_register() seems really
PeterC> strange...

>> *(int *)&devinfo->speed = CDROM_STATE_FLAGS
>> (drive)->current_speed; *(int *)&devinfo->capacity = nslots;

PeterC> devinfo-> speed and devinfo->capacity are both ints.  So the casts are
PeterC> just a disaster waiting to happen, if the types of capacity or
PeterC> speed ever change?

Michael> Just take a quick look in drivers/ide/ide-cd.h: values
Michael> "nslots" and "current_speed" are of type "byte", so we need
Michael> to cast to store them (like that) into the
Michael> integer-vars. Nothing strange there....

Sure, the RHS is a byte.  But devinfo->speed is an int and an lvalue. so
&devinfo->speed is an (int*) (so the cast in this case is a no-op),
and *(int*)&devinfo->speed is the same as *&devinfo->speed which is
the same as devinfo->speed.

But, *(int*)&devinfo->speed is an int no matter what type
devinfo->speed has.  So if for some reason, you decide to change the
type of devinfo->speed to a byte, say, the cast will still force
int format data to be stored, overwriting adjacent bits of memory (or
causing an unaligned store trap).

The cast is *wrong*, and potentially dangerous.

I'll submit a patch....

--
Peter C					    peterc@gelato.unsw.edu.au
You are lost in a maze of BitKeeper repositories, all almost the same.

Re: Strange code in ide_cdrom_register

[J.A. Magallon]

On 2002.05.30 Peter Chubb wrote:
>>>>>> "Michael" == Michael Dunsky <michael.dunsky@p4all.de> writes:
>
>But, *(int*)&devinfo->speed is an int no matter what type
>devinfo->speed has.  So if for some reason, you decide to change the

Perhaps this comes from a time when devinfo->speed was a void*.
I remember changes of that kind on the IDE layer commented in the list.

-- 
J.A. Magallon                           #  Let the source be with you...        
mailto:jamagallon@able.es
Mandrake Linux release 8.3 (Cooker) for i586
Linux werewolf 2.4.19-pre9-jam1 #1 SMP mié may 29 02:20:48 CEST 2002 i686

Re: Strange code in ide_cdrom_register

[Michael Dunsky]

Hi!

Peter Chubb wrote:
....
  > The cast is *wrong*, and potentially dangerous.
  >
  > I'll submit a patch....

OK. Maybe my problem is this
(in thinking - last night was definetly too short...):

---------- from ide-cd.c ------------------
static int ide_cdrom_register (ide_drive_t *drive, int nslots)
{
     struct cdrom_info *info = drive->driver_data;
     struct cdrom_device_info *devinfo = &info->devinfo;
     ...
     *(int *)&devinfo->speed = CDROM_STATE_FLAGS (drive)->current_speed;
     *(int *)&devinfo->capacity = nslots;

---------- from ide-cd.c ------------------

As you can see there are several stages of pointers:
Parameter "drive" is pointer to the original var,
"info" is a pointer to "drive->driver_data",
"devinfo" is a pointer to the address of "info->devinfo".

So we put a value into a mem-address referenced by several pointers -
but whats the type of that address? The other values are (nearly all)
just simply ints or pointers. Just putting a byte-value into a field
defined as int would probably be wrong.

But, Russel, you're right:
If we had to cast we would do it with the source.
This _is_ strange code *scratch head*  :-/

ciao

Michael