Why are these 2 code blocks not the same?

[Paul McKenzie]

As a matter of fact, what is this in your code?

smpl

You pass this as an output buffer. Nowhere do you state where you've initialized this, or what this is supposed to be. This is probably where things start to break down.

Regards,

Paul McKenzie

[mmscg]

I really appreciate the help!!

smpl is a pointer to an output buffer


This is very hard to debug using the debugger because the inner loop gets executed approx 510 times,
and I have to respond to 510 breakpoints (unless I am still not using the debugger properly).

Is there a way to print to a text file so I can review the variables at each line in the loop.
(I could just print the last 10 executions of the loop as it
seems it is not until this point that sectorsPerRead starts changing)

[Paul McKenzie]

I really appreciate the help!!

smpl is a pointer to an output buffer

We all know it's a pointer to an output buffer.

Please, can you be a little more specific? Where is it initialized? What is it declared as? Where does it point to initially? Where is this "buffer" that it points to?

It isn't a matter of the debugger, it is a matter of understanding what you've programmed. When an API function asks for a pointer to a buffer, for the most part, the API function is assuming you are pointing to somewhere that is valid, and is big enough to hold the information when the function is executed.

If you just declare an LPVOID or whatever, and do not point it anywhere, of course you'll have memory overwrites leading to weird behaviour and crashes.

Regards,

Paul McKenzie

[Paul McKenzie]

This is very hard to debug using the debugger because the inner loop gets executed approx 510 times,

Why not change your program so that you create your own breakpoint when the code doesn't act correctly?

// read data (samples) from CD
        ULONG testValue = sectorsPerRead;

        fSuccess = DeviceIoControl(pextr->hCDROM,          
                                   IOCTL_CDROM_RAW_READ,     
                                   &info,                  
                                   sizeof(RAW_READ_INFO),  
                                   smpl,                   
                                   sectorsPerRead * RAW_SECTOR_SIZE,            
                                   &bytesRead,             
                                   0);                     

        if ( testValue != sectorsPerRead )
        {
               // what happened?
               testValue = sectorsPerRead  // put a breakpoint here
        }
}

Very simple. Just create code where you know it will only be reached if there is a problem, and put a breakpoint in that code.

But really, the problem is more than likely your output buffer. Please, next time when code is posted, if you are going to use variables, post where they came from, how they are initialized, etc. Variables that just appear out of thin air in code snippets are notorious for being the cause of problems (for example, the "smpl" variable).

Regards,

Paul McKenzie

[mmscg]

PUCHAR    smpl;
smpl = pextr ->pmem _ sizeof(CDROM_TOC) + RAW_SECTOR_SIZE;

I don't think sectorsPerRead is in fact a problem
(other than the fact that it gets changed somewher and I don't know why or how).

As I said this code works:

        // read data (samples) from CD
        fSuccess = DeviceIoControl(pextr->hCDROM,          
                                   IOCTL_CDROM_RAW_READ,     
                                   &info,                  
                                   sizeof(RAW_READ_INFO),  
                                   smpl,                   
                                   sectorsPerRead * RAW_SECTOR_SIZE,            
                                   &bytesRead,             
                                   0);                     
                
        if(bytesRead != sectorsPerRead * RAW_SECTOR_SIZE) __leave;

This does not:
(so it makes sense that bytesRead does not equal bytesPerRead and the inner loop is exited
before it finishes ripping CD data)

        bytesPerRead = sectorsPerRead * RAW_SECTOR_SIZE;

        // read data (samples) from CD
        fSuccess = DeviceIoControl(pextr->hCDROM,          
                                   IOCTL_CDROM_RAW_READ,     
                                   &info,                  
                                   sizeof(RAW_READ_INFO),  
                                   smpl,                   
                                   bytesPerRead,            
                                   &bytesRead,             
                                   0);                     
                
        if(bytesRead != bytesPerRead) __leave;

I can understand everyones frustration with me,
but believe me I am trying to figure this out on my own and not waste forum space.

Why not change your program so that you create your own breakpoint when the code doesn't act correctly?

Awesome suggestion!!

[Paul McKenzie]

PUCHAR    smpl;
smpl = pextr ->pmem _ sizeof(CDROM_TOC) + RAW_SECTOR_SIZE;

According to the documentation for DeviceIoControl, the buffer that you pass to it must be big enough to hold the information that will be written. What you posted there doesn't mean anything to anyone looking at your code, i.e. it doesn't convey how big the buffer is. The bottom line is this.

Can you guarantee that the buffer that is being pointed to is

1) valid (no one can tell by looking at your code)

AND

2) large enough to hold the information returned by DeviceIoControl (again, no one can tell by looking at your code)

If either 1) or 2) are not true, then your code is faulty.

Why not do something simple -- make smpl point to a buffer that is 10,000 bytes and forget about all of that other stuff temporarily?

UCHAR    smpl[10000];
//smpl = pextr ->pmem _ sizeof(CDROM_TOC) + RAW_SECTOR_SIZE;

Now, does the loop perform more times? Does the memory overwrite occur? If it doesn't occur, then the problem is the output buffer you were providing to DeviceIoControl -- it was either too small, or it was pointing to somewhere invalid (or both).

I don't think sectorsPerRead is in fact a problem
(other than the fact that it gets changed somewher and I don't know why or how).

I think I've been trying to tell you the why and how this can happen. Your call to DeviceIoControl is more than likely faulty because of a memory overwrite.

As I said this code works:

We know what works and what doesn't. We are trying to convey to you why it doesn't work and what is the probable cause. Anyone who has experience in the language will tell you that the call to DeviceIoControl, given what all of those parameters mean and what DeviceIoControl does, is what you should be really inspecting.

Regards,

Paul McKenzie

[mmscg]

Can you guarantee that the buffer that is being pointed to is

1) valid (no one can tell by looking at your code)

AND

2) large enough to hold the information returned by DeviceIoControl (again, no one can tell by looking at your code)

If either 1) or 2) are not true, then your code is faulty.

I am 99% sure that the buffer is both valid and large enough to hold return info from DeviceIoControl call.

Why not do something simple -- make smpl point to a buffer that is 10,000 bytes and forget about all of that other stuff temporarily?

I will perform this test tonight.

Now, does the loop perform more times? Does the memory overwrite occur? If it doesn't ...

I'm not sure what you mean by the memory overwrite?

[Paul McKenzie]

I am 99% sure that the buffer is both valid and large enough to hold return info from DeviceIoControl call

99% percent sure is not a guarantee. You must be 100% sure that the buffer is large enough to hold the information, otherwise the code has to be rewritten. Also, it's hard for us helping you to say "OK" to what you're saying. Unless we have the code ourselves, or could decipher what all of that was that you posted, the buffer being too small is still on the table as the reason for the problem.

I'm not sure what you mean by the memory overwrite?

#include <string.h>
void MyOwnDeviceIoControl(char *pOut, int* pOutLength)
{
     int OutputLength = 100;
     memset(pOut, 0, OutputLength);  // memory overwrite
    *pOutLength = 100;
}

int main()
{
     int xyz = 149;
     char whatever[10];
     int length;
     MyOwnDeviceIoControl(whatever, &length);
     // xyz has changed
}

I ran this on Visual C++ 6.0. The value of xyz is 149. However, after the call to MyOwnDeviceIoControl is made, xyz has mysteriously changed to 0.

This is exactly the scenario that you're describing. The "whatever" buffer was too small, since the MyOwnDeviceIoControl function assumes that you are pointing to a buffer that can hold at least 100 bytes. When the function does the memset() it writes 100 bytes starting at the "whatever", and like a bull in a china shop, overwrites anything in its way, including the xyz variable. This is a classic memory overwrite.

Of course, the code may crash and burn on the memset(), but this is an illustration of why I believe your buffer is not big enough, even though you say it is.

Regards,

Paul McKenzie

[angelorohit]

I think you may need to rewrite the code from scratch. There are quite a few inconsistencies in the way you used DeviceIoControl() and the way msdn declares it. The types used also do not match as pointed out by Paul.

1. if info is the input buffer, then the next argument should be sizeof(info) instead of sizeof(RAW_READ_INFO). If RAW_READ_INFO has been defined as a macro then sizeof(RAW_READ_INFO) will be 4 bytes.

2. Likewise, I'm not sure about your output buffer. Are you certain that sectorsPerRead * RAW_SECTOR_SIZE is the size of the output buffer? Or for that matter is bytesPerRead?

3. Finally, if you think about it, the program goes on reading data from the CD and filling the output buffer with the data. Ultimately, when it comes to the last block of data, the output buffer may not be completely filled, in which case bytesRead will not be equal to bytesPerRead. In other words, this is not a proper way to check whether data has been read or not. To see the proper way, refer this link http://msdn2.microsoft.com/en-us/library/aa363216.aspx

...and like a bull in a china shop, overwrites anything in its way...

Very Effectively put.

I can understand everyones frustration with me,
but believe me I am trying to figure this out on my own...

We believe you. Your frustration has become our frustration. Unlike some of the other lazy programmers out there, at least you are trying to figure it out. That's commendable.

[mmscg]

Why not change your program so that you create your own breakpoint when the code doesn't act correctly?

if ( testValue != sectorsPerRead )
{
// what happened?
testValue = sectorsPerRead // put a breakpoint here
}

If statement never executed.


So I printed out variables at each loop iteration (code as posted loops 508 times).

... sectorsPerRead automagically changes from 27 (as I had assigned it) to 6 and then 1

   i    bytesRead    bytesPerRead    sectorsPerRead *    sectorsPerRead    fSuccess
                                     RAW_SECTOR_SIZE
----    ---------    ------------    ----------------    --------------    --------
   .
   .
   .
 501        63504           63504               63504                27           0
 502        63504           63504               63504                27           0
 503        63504           63504               63504                27           0
 504        63504           63504               63504                27           0
 505        14112           63504               14112                 6           0
 506         2352           63504                2352                 1           0
 507         2352           63504                2352                 1           0
 508         2352           63504                   0                 0           0

2352 X 27 = 63504
2352 X 6 = 14112
2352 X 1 = 2352

Why not do something simple -- make smpl point to a buffer that is 10,000 bytes and forget about all of that other stuff temporarily?

I did this and now code loops 4,549 times .

I changed the buffer size to 10,000 bytes, but changed nothing else
(sectorsPerRead is left equal to 27).

Printing out the variables at each loop iteration now yields:

(and sectorsPerRead now automagically becomes 3)

   i    bytesRead    bytesPerRead    sectorsPerRead *    sectorsPerRead    fSuccess
                                      RAW_SECTOR_SIZE
----    ---------    ------------    ----------------    --------------    --------
   .
   .
   .
4542         7056           63504                7056                 3           0
4543         7056           63504                7056                 3           0
4544         7056           63504                7056                 3           0
4545         7056           63504                7056                 3           0
4546         7056           63504                7056                 3           0
4547         7056           63504                7056                 3           0
4548         7056           63504                7056                 3           0
4549         2352           63504                   0                 0           0

2352 X 3 = 7056


angelorohit:

I think you may need to rewrite the code from scratch.

I think you are right.

2. Likewise, I'm not sure about your output buffer. Are you certain that sectorsPerRead * RAW_SECTOR_SIZE is the size of the output buffer? Or for that matter is bytesPerRead?

Yes, see above data printouts.

3. Finally, ... ...To see the proper way, refer this link http://msdn2.microsoft.com/en-us/library/aa363216.aspx

The link is invalid.



Again, thanks all for the help!!

[ZuK]

Since you still refuse to show us the definitions of your buffer and your variable sectors_per_read I think you still haven't understood what everybody here thinks is happening in your program.
I give you some example

#include <iostream>
#include <string>
#include <iomanip>

using namespace std;

int main() {

    int sectors_per_read = 100;
    char buffer[15];

    cout << "before out of bounds assignement" << endl;
    cout << left << setw(20) << "sectors_per_read " << sectors_per_read << endl;


    // overflow buffer
    // something like this might happen with your DeviceIoControl() call if you 
    // specify a wrong buffer size.
    for ( int i= 0; i < 32; ++i )
        buffer[i] = 1;

    cout << "after out of bounds assignement" << endl;
    cout << left << setw(20) << "sectors_per_read " << sectors_per_read << endl;
}

my output

before out of bounds assignement
sectors_per_read    100
after out of bounds assignement
sectors_per_read    16843009

Your compiler might allocate different amount of memory for that variables, so this example might crash on your system or it might appear to work correctly.
Hope you understand now how it's possible that variables change their value automagically.
Kurt

[Griffin49]

You'll probably find that the best search tool for programming is Google. Google seem to have indexed all of Microsoft's help pages, vast numbers of newsgroups, and all of sites like CodeGuru.

And then, if Google doesn't turn up a useful answer, I'd search other sources.

[Griffin49]

Most modern debuggers let you set conditions on the breakpoint so you don't have to respond to the first 'n' iterations throught loops, etc. If you have a loop counter, or some other variable whose state indicates when you really want to break, have the condition test it for the trigger value.

[mmscg]

I've figured it out!!

Well actually angelorohit did.

In trying to understand how the exact amount of audio sectors gets copied to hard drive, when audio will not always be a multiple of 27,
it hit me that that was the purpose of the outer loop - to adjust sectorsToRead to compensate for this in the last few loop iterations.

angelorohit tried to tell me that way back in post #15

This is the line that is changing the value of sectorsPerRead in the outer loop. Essentially, it shifts the bits of sectorsPerRead by one towards the right. This is a fast and efficient way of dividing by 2. Its the same as saying :
Code:
sectorsPerRead /= 2;

When I try to read more sectors than are actually left in the audio being copied,
the inner loop is exited, and the sectorsPerRead is adjusted to [b]half the previous amount (27/2=13),
it checks again, and again 13 is more than the sectors left so
inner loop is exited again and sectorsPerRead is again adjusted to half the previous amount (13/2=6),
this time it is able to read & copy 6 sectors,
This is repeated until all the remaining sectors have been read.

I won't be able to verify this 'till later, but I'm certain this is what is happening.

Thanks for all the help!!

[angelorohit]

Ah, finally! Glad to hear I could be of some assistance. You see? My version of Murphy's law states that the offending statement will be the one that you don't understand the most. That is why I spoke about the outer loop and the shift operator - to make sure that you know what's happening.