C++ Peer Review requested [closed]

Question

Pre-PS: This was originally posted here. Thanks to Paul C. for pointing me in the right direction!

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()

Answer 1

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.