Dead code in dlls/ntdll/tape.c

[Gerald Pfeifer]

We currently have the following code in tape.c:

if (data->Offset.u.LowPart >= 0) {
            cmd.mt_op = MTFSF;
            cmd.mt_count = data->Offset.u.LowPart;
        }
        else {
            cmd.mt_op = MTBSF;
            cmd.mt_count = -data->Offset.u.LowPart;
        }

data is of type TAPE_SET_POSITION*, which in turn is defined as

typedef struct _TAPE_SET_POSITION {
    ULONG Method;
    ULONG Partition;
    LARGE_INTEGER Offset;
    BOOLEAN Immediate;
  } TAPE_SET_POSITION, *PTAPE_SET_POSITION;

Offset is of type LARGE_INTEGER which is defined as

struct {
        DWORD    LowPart;
        LONG     HighPart;
    } u;

Note how LowPart is unsigned (DWORD) here, so indeed the comparisons
for >= 0 is always going to evaluate to true.

Hans, I believe you originally wrote that code.  Do you remember what
you where trying to do here?

The patch below removes this dead code and is equivalent to what we
currently have.  I am not sure it is the desired way to fix what I
describe above, though.

Gerald

Index: dlls/ntdll/tape.c
===================================================================
RCS file: /home/wine/wine/dlls/ntdll/tape.c,v
retrieving revision 1.16
diff -u -3 -p -r1.16 tape.c
--- dlls/ntdll/tape.c	26 Jun 2007 12:14:34 -0000	1.16
+++ dlls/ntdll/tape.c	1 Nov 2007 16:32:29 -0000
@@ -427,25 +427,13 @@ static NTSTATUS TAPE_SetPosition( int fd
         break;
 #endif
     case TAPE_SPACE_FILEMARKS:
-        if (data->Offset.u.LowPart >= 0) {
-            cmd.mt_op = MTFSF;
-            cmd.mt_count = data->Offset.u.LowPart;
-        }
-        else {
-            cmd.mt_op = MTBSF;
-            cmd.mt_count = -data->Offset.u.LowPart;
-        }
+        cmd.mt_op = MTFSF;
+        cmd.mt_count = data->Offset.u.LowPart;
         break;
 #if defined(MTFSS) && defined(MTBSS)
     case TAPE_SPACE_SETMARKS:
-        if (data->Offset.u.LowPart >= 0) {
-            cmd.mt_op = MTFSS;
-            cmd.mt_count = data->Offset.u.LowPart;
-        }
-        else {
-            cmd.mt_op = MTBSS;
-            cmd.mt_count = -data->Offset.u.LowPart;
-        }
+        cmd.mt_op = MTFSS;
+        cmd.mt_count = data->Offset.u.LowPart;
         break;
 #endif
     case TAPE_LOGICAL_BLOCK:

[Hans Leidekker]

On Thursday 01 November 2007 21:22:14 Gerald Pfeifer wrote:
> We currently have the following code in tape.c:
>     if (data->Offset.u.LowPart >= 0) {
>         cmd.mt_op = MTFSF;
>         cmd.mt_count = data->Offset.u.LowPart;
>     }
>     else {
>         cmd.mt_op = MTBSF;
>         cmd.mt_count = -data->Offset.u.LowPart;
>     }
> Offset is of type LARGE_INTEGER which is defined as
> struct {
>     DWORD    LowPart;
>     LONG     HighPart;
> } u;
>
> Note how LowPart is unsigned (DWORD) here, so indeed the comparisons
> for >= 0 is always going to evaluate to true.

Yes, the comparison should be against data->Offset.QuadPart (which you
left out of the definition of LARGE_INTEGER).

I'll work on a fix.

-Hans

[Robert Shearman]

Gerald Pfeifer wrote:
> We currently have the following code in tape.c:
>     if (data->Offset.u.LowPart >= 0) {
>         cmd.mt_op = MTFSF;
>         cmd.mt_count = data->Offset.u.LowPart;
>     }
>     else {
>         cmd.mt_op = MTBSF;
>         cmd.mt_count = -data->Offset.u.LowPart;
>     }
>
> data is of type TAPE_SET_POSITION*, which in turn is defined as
>
> typedef struct _TAPE_SET_POSITION {
>     ULONG Method;
>     ULONG Partition;
>     LARGE_INTEGER Offset;
>     BOOLEAN Immediate;
>   } TAPE_SET_POSITION, *PTAPE_SET_POSITION;
>
> Offset is of type LARGE_INTEGER which is defined as
> struct {
>     DWORD    LowPart;
>     LONG     HighPart;
> } u;
>
> Note how LowPart is unsigned (DWORD) here, so indeed the comparisons
> for >= 0 is always going to evaluate to true.
>
> Hans, I believe you originally wrote that code.  Do you remember what
> you where trying to do here?
>
> The patch below removes this dead code and is equivalent to what we
> currently have.  I am not sure it is the desired way to fix what I
> describe above, though.

The LARGE_INTEGER type without #ifdefs for clarity:
typedef union _LARGE_INTEGER {
    struct {
        DWORD    LowPart;
        LONG     HighPart;
    } u;
    LONGLONG QuadPart;
} LARGE_INTEGER, *PLARGE_INTEGER;

The type is meant to be wrap a signed integer, but the compiler 
legitimately flagged that it isn't in the way that it is being accessed 
currently. Therefore, I believe the code should be changed to this:
>     if (data->Offset.QuadPart >= 0) {
>         cmd.mt_op = MTFSF;
>         cmd.mt_count = (int)data->Offset.QuadPart;
>     }
>     else {
>         cmd.mt_op = MTBSF;
>         cmd.mt_count = -(int)data->Offset.QuadPart;
>     }