Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This was originally posted here.

I was hired to build a console app in C++ that uses the WinAPI CreateFile function to open a hard disk, read some data, modify some data, and write some data.

Per the "employer's" specs, this product takes a minimum 2 and maximum 4 arguments, does its thing, and writes a brief log. Argument 0 is the target disk index (index to command is same as index from Computer Management MMC snapin), Argument 1 is target disk sector index (assuming a 512B sector). The employer alleges that whenever he puts in a big number (a 64-bit sector index) something is truncating it to 32 bits - he gets this misconception from a 3rd party product called BAM which (he thinks) is monitoring SATA ports by monitoring the PCI BUS - whether he's right or wrong about how BAM works, it's outputting a 32-bit sector index that represents the same byte offset into the disk if we assumed sector sizes were 2048B (he insists that all sector sizes, everywhere and for all time, are 512B).

I've told him repeatedly to check the disk using a hex editor that will open a disk, which he claims he's done, but I've verified functionality myself the same way so I'm pretty sure he's wrong. So I'm asking for someone to compile the code, test it against a raw drive (one without partitions) using a 64-bit sector index (any sector number over 5GB will do, but over 1TB preferred and over 2TB even better), review the drive with a hex editor, and report back. The product will fail in Vista and higher if used against a partitioned disk.

The following code compiles using the Visual Studio Command Prompt and the commandline: cl /EHsc writeLBA.cpp Advapi32.lib

 #pragma warning(disable:4786)
 #pragma warning(disable:4996)

 #include <windows.h>
 #include <Winbase.h>
 #include <Lmcons.h>
 #include <iostream>
 #include <fstream>
 #include <ctime>

 using namespace std ;

 char insDat[] = "Hello LBA!";

 char logPath[] = ".\\writeLBA.log";
 ofstream logFile;

 char diskPre[]  = "\\\\.\\PhysicalDrive";
 char diskPath[21];

 char exit_Msg[] = "Command completed successfully.";
 int  exit_Val   = 0;

 char exit_ErrorArgsCountMsg[] = "Wrong number of arguments.";
 int  exit_ErrorArgsCountVal   = -1;

 char exit_ErrorDiskIDMsg[]    = "Disk ID must be 0 to 255.";
 int  exit_ErrorDiskIDVal      = -2;

 char exit_ErrorDiskIOMsg[]    = "Disk IO failed at CreateFile call.";
 int  exit_ErrorDiskIOVal      = -3;

 char exit_ErrorSecIOMsg[]    = "Sector IO failed.";
 int  exit_ErrorSecIOVal      = -4;

 char exit_ErrorWriteIOMsg[]  = "Disk writeback failed.";
 // exit_ErrorWriteIOMsg Error value will be rewritten by GetLastError value if any exists.

 void writeLog(int argc,char* argv[])
 {
     logFile.open(logPath, fstream::in | fstream::out | fstream::app);

     TCHAR uName[UNLEN+1];
     DWORD uNameLen = sizeof(uName);
     int apiRet = GetUserNameA(uName,&uNameLen);

     DWORD diskSer = 0x00000000;
     apiRet = GetVolumeInformation(diskPath,0,0,&diskSer,0,0,0,0);


     time_t now = time(0);
     tm *ltm = localtime(&now);
     logFile << 1900 + ltm->tm_year << "/" << 1 + ltm->tm_mon << "/" <<  ltm->tm_mday << ",";
     logFile << ltm->tm_hour << ":" << ltm->tm_min << ":" << ltm->tm_sec << ",";
     logFile << diskSer << ",";
     logFile << uName << ",";
     logFile << diskPath << ",";
     logFile << exit_Msg << ",";
     if (argc > 3)
     {
         logFile << argv[3];
     } // if (argc > 3)
     logFile << ",";
     if (argc > 4)
     {
         logFile << argv[4];
     } // if (argc > 4)
     logFile << "\n";

     logFile.close();
 } // void writeLog()

 int main(int argc,char* argv[])
     {
         if ((argc < 3) || (argc > 5))
         {
             strcpy(exit_Msg,exit_ErrorArgsCountMsg);
             writeLog(argc,argv);
             cout << exit_Msg;
             return exit_ErrorArgsCountVal;
         } // if ((argc < 3) || (argc > 4))

         int diskId = atoi(argv[1]);
         if ((diskId < 0) | (diskId > 255))
         {
             strcpy(exit_Msg,exit_ErrorDiskIDMsg);
             writeLog(argc,argv);
             cout << exit_Msg;
             return exit_ErrorDiskIDVal;
         } // if ((diskId < 0) | (diskId > 255))

         strcpy(diskPath,diskPre);
         strcat(diskPath,argv[1]);

         HANDLE hDiskIO = CreateFileA(diskPath,
                                     GENERIC_READ | GENERIC_WRITE,
                                     FILE_SHARE_READ | FILE_SHARE_WRITE,
                                     0x00000000,
                                     OPEN_EXISTING,
                                     FILE_ATTRIBUTE_NORMAL,
                                     0x00000000
                                     );

         if (hDiskIO == INVALID_HANDLE_VALUE)
         {
             strcpy(exit_Msg,exit_ErrorDiskIOMsg);
             writeLog(argc,argv);
             cout << exit_Msg << " for " << diskPath;
             return exit_ErrorDiskIOVal;
         } // if (hDiskIO <= 0)

         unsigned __int64 secID = atol(argv[2]);
         secID = secID * 512; // convert the user's sector index to a byte offset into the disk
         unsigned long distL = (unsigned long)((secID << 32) >> 32);
         long distH = (long)(secID >> 32);

         int apiRet = SetFilePointer(hDiskIO,
                                     distL,
                                     &distH,
                                     FILE_BEGIN
                                    );

         if (apiRet == INVALID_SET_FILE_POINTER)
         {
             strcpy(exit_Msg,exit_ErrorSecIOMsg);
             writeLog(argc,argv);
             cout << exit_Msg << "  Unable to seek to sector " << argv[2];
             return exit_ErrorSecIOVal;
         } // if (apiRet == INVALID_SET_FILE_POINTER)

         char datBuf[512];
         DWORD bRead = 0x00000000;
         apiRet = ReadFile(hDiskIO,
                           &datBuf,
                           512,
                           &bRead,
                           0
                          );

         apiRet = SetFilePointer(hDiskIO,
                                 distL,
                                 &distH,
                                 FILE_BEGIN
                                );

         // To copy the string value of insDat sans 0x00 terminator, use sizeof()-1 for copy length
         memcpy(&datBuf,&insDat,sizeof(insDat) - 1);

         // Clear any prior error condition unset by prior code success
         SetLastError(0x00000000);

         // UNCOMMENT THE NEXT 6 LINES TO PERFORM WRITEBACK
         apiRet = WriteFile(hDiskIO,
                            &datBuf,
                            sizeof(datBuf),
                            &bRead,
                            0
                           );

         if (apiRet == 0)
         {
             apiRet = GetLastError();
             strcpy(exit_Msg,exit_ErrorWriteIOMsg);
             exit_Val = apiRet;
             writeLog(argc,argv);
             cout << exit_Msg << " for " << diskPath;
             return exit_Val;
         } // if (apiRet == 0)

         apiRet = CloseHandle(hDiskIO);

         writeLog(argc,argv);
         cout << exit_Msg;
         return exit_Val;
     } // int main()
share|improve this question
3  
Welcome to Code Review, and thanks for posting your code! Please don't expect members of the community to test your code in complex scenarios; rather, expect people to rip apart your code and point out everything they think is wrong with it. Anything apart from the review of working code is off-topic here, as defined in the faq. –  codesparkle Feb 12 '13 at 18:28
4  
Sorry; if you felt I was overlay harsh. But this is the kind of review you should expect in any organization that does code review. Admittedly some is overkill for such a short piece of code but the point is to emphasize good habits (which you do need to practice). But if you read carefully you will find at least one bug that makes the code broken. And no I am not going to run your code (apart from it being broken it has the potential for really screwing up the disk (if I miss something else)). –  Loki Astari Feb 12 '13 at 19:01
add comment

closed as off topic by codesparkle Feb 12 '13 at 22:56

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

Don't write comments that mimic the code.
Comments should explain why you are doing something not how (the code explains how). I would consider most of your comments useless and some are even wrong (which is why you should not try and explain the code with comments (the code explains the code)).

     // This comment is wrong.
     // So it is worse than useless; it will cause problems
     // down the road as any maintainer will have to work
     // out if the code is wrong or the comment is wrong.
     // Thus wasting time and money.

     } // if ((argc < 3) || (argc > 4))

Don't do this:

using namespace std ;

I won't go over this again as it is repeated in every review about C++. You can read a great answer about it here: http://stackoverflow.com/a/1453605/14065

It is not hard to prefix stuff with std:: so please do so.

In every error situation you copy the error msg over the default message then print it. I don;t see the point and it is dangerious:

 strcpy(exit_Msg,exit_ErrorArgsCountMsg);

You provide no checking that exit_ErrorArgsCountMsg will fit in exit_Msg. It just happens to be that way. Any new error message that is added needs to be manually checked by the programmer (this is bad design). In the end there seems to be no reason to do this as all that is done by the variable is that it is printed out then the program exits.

I would reformat all the error messages into a single array of C-strings:

 char const exit_Msg[5][] = {
                       "Command completed successfully.",
                       "Wrong number of arguments.",
                       "Disk ID must be 0 to 255.".
                       "Disk IO failed at CreateFile call.",
                       "Sector IO failed.",
                       "Disk writeback failed."
                      };

Then your error case just becomes:

    if (/*Whatever your error condition test is: */)
    {
         writeLog(argc,argv);
         cout << exit_Msg[/*Error Number*/] << "\n";
         return - /*Error Number*/;
    }

Why is this a global variable?

ofstream logFile;

Keep variable declarations as close to their use point as possible. Prefer never to have global state (ie this means prefer to not to have "non cost" global variables). This ties any function/method to this global state and makes unit testing harder.

What happens if argv[2] is not a number?

unsigned __int64 secID = atol(argv[2]);

This is rather an obtuse method of setting the top 32 bits to 0.

unsigned long distL = (unsigned long)((secID << 32) >> 32);

If you just want to knock of the top bits then a bit-wise and is the usual method.

unsigned long distL = secID & 0xFFFFFFFFLL;

That distL and distH have different types is strange. So I looked up the API for SetFilePointer(). Is it happens they are both LONG. You should declare your types appropriately.

Why don't you check the error condition here:

     apiRet = ReadFile(hDiskIO,
                       &datBuf,  // This looks very wrong.
                                 // datBuf will already decay to a pointer
                                 // maybe you meant `&datBuf[0]`
                       512,
                       &bRead,
                       0         // Yes 0 works.
                                 // But NULL provides information on intent.
                                 // You are explicitly providing a NULL pointer
                                 // Not accidentally passing the flag field with the value zero.
                      );

Or here:

     apiRet = SetFilePointer(hDiskIO,
                             distL,
                             &distH,
                             FILE_BEGIN
                            );

Do you think this comment provides extra clarity?

     // Clear any prior error condition unset by prior code success

This comment is unhelpful. Especially since it is wrong and the code is not commented out:

     // UNCOMMENT THE NEXT 6 LINES TO PERFORM WRITEBACK

You only close the file handle if everything worked:

apiRet = CloseHandle(hDiskIO);

As soon as the file is validated as opened correctly. I would then try and guarantee that the file is always closed (via a call to CloseHandle()) no matter what happens in the code. Read up on RAII.

share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.