[ros-kernel] scsi/disk code comments [was: IOMega Zip Drive]

[Mike Nordell]

Commenting some quoted code, just because the opportunity presented itself.

James Tabor wrote:

> In drivers/storage/class2.c line ~1770,
[snip]
>    if (IrpStack->Parameters.DeviceIoControl.OutputBufferLength <
>        sizeof(DISK_GEOMETRY))
>    {
>        Status = STATUS_INVALID_PARAMETER;
>        break;
>    }

This is wrong. I get, as I would have expected, Win32
ERROR_INSUFFICIENT_BUFFER from the DeviceIoControl call if I give it a too
small output buffer.

***
Note to all: Please notice the difference between giving a function
explicitly bad parameters and "just" a too small output buffer, and report
these conditions accordingly. Thanks.
***

>          if (DeviceExtension->DiskGeometry == NULL)
>            {
>              DPRINT1("No disk geometry available!\n");
>              DeviceExtension->DiskGeometry = ExAllocatePool(NonPagedPool,
>
sizeof(DISK_GEOMETRY));

Perhaps I'm giving a too great attention to details, but since the next line
is allocating it, perhaps something like "Allocating new disk geometry for
device" instead? (should "device" be able to be identified by a human
readable string, I'd prefer even that appended)

>          if (DeviceObject->Characteristics & FILE_REMOVABLE_MEDIA)
>            {
>              Status = ScsiClassReadDriveCapacity(DeviceObject);
>              DPRINT1("ScsiClassReadDriveCapacity() returned (Status
%lx)\n", Status);
>              if (!NT_SUCCESS(Status))
>                {
>                  /* Drive is not ready */
>                  DPRINT1("DRIVE NOT READY G.1\n");
>                  DiskData->DriveNotReady = FALSE;

The context led me to read this as SCSI_SENSE_NOT_READY. :-)
But please see the next comment.

>              /* Drive is ready */
>                  DPRINT1("DRIVE IS READY ?G.2\n");
>              DiskData->DriveNotReady = TRUE;

Isn't it interesting how negations can make things seem to be what they are
not. Like, the completely opposite? :-)

If the boolean member is called "DriveNotReady" I would expect it to be TRUE
if the drive is NOT ready, and FALSE otherwise. I suggest that either the
TRUE/FALSE are flipped above, or the member renamed to something without
negation.

As a friend used to have his .sig
"Somethings are not things like other things.
 Somethings aren't not things like other things.
 These things are not the same things, usually."

> The assumption is wrong, not all drives think this way.

Would you care to elaborate (disregarding the flipped NotReady logic)?

> ..And this from disk.c line ~841 for reference,
>
>        case IOCTL_DISK_GET_DRIVE_GEOMETRY:
>          DPRINT1("IOCTL_DISK_GET_DRIVE_GEOMETRY\n");
>          if (IrpStack->Parameters.DeviceIoControl.OutputBufferLength <
sizeof(DISK_GEOMETRY))
>            {
>              Status = STATUS_INVALID_PARAMETER;

Again I suspect my comment above about reporting errors could hold some
ground, though I'm not 100% sure in this context.

But... isn't this about the same code as in class2.c? Why the (seemingly)
copy'n'paste duplication?

[snip]

> !*!*!*! Here for some reason it set DriveNotReady FALSE,

Didn't it do the same thing in class2.c?

Could someone also explain why this code duplication is required in the
first place, I'd appreciate it.


/Mike