pointers, bool types, why doesn't this work?

[Dan Feerst]

This tiny program is supposed to accept a hard drive as a device,
read the bootsignature and verify that it is 0x55AA
So far, the bootSignature pointer points to the first byte.
I am having trouble with the bool statement, what am I doing wrong?
also, as I'm sure you will notice I am only referencing the first byte. What is the best way to reference both. would it be some type of array? I tried to get one working without any luck.

#include <iostream>
#include <fstream>
using namespace std;
typedef unsigned int uint;
struct mbr_t{
	char * bootSignature;      //2 bytes
	bool * bootSignatureValid;
	}mbr;

	
void isValid(char &c, const char C, bool &b){
	    if( c == C ) b = true;
	    else b = false;
	}

int main(int argc, char** argv){
  size_t eof;  
  ifstream datafile ( argv[1] , ios::binary);
  char * buf = new char[eof];  // Allocate a buffer for the file 
  datafile.read (buf, eof);  // Read the file into the buffer
  mbr.bootSignature = (char*)(buf + 510);
  isValid(*mbr.bootSignature,0x55, *mbr.bootSignatureValid);
  if(mbr.bootSignatureValid){
	  cout << "yep"<<endl;
	  }
  else cout <<"nope"<<endl;
}

[Disch]

I don't know what the main problem is... it helps if you actually describe the problem instead of just saying you're "having trouble".


What I do see, however, is some very bad (program breaking) practices.

Namely:

#1: You never initialize your 'eof' variable on line 17. So when you allocate memory on line 19... how much are you allocating? You never specify. It could be 1 byte or it could be a billion. How much you actually allocate -- the contents of your 'eof' var -- is garbage because you never assign it.

#2: You are using new[] but you are not using delete[] to clean up. Memory leak.

#3: Your isValid function is kind of a mess. It makes much more sense to just return the output rather than pass by value:

bool isValid(char c, char C)  // no need to pass by const or by reference
{
    return (c == C);  // return true or false... depending on whether they match
}

// then to use it... instead of doing this:
//   isValid(*mbr.bootSignature,0x55, *mbr.bootSignatureValid);
// You can do this:
*mbr.bootSignatureValid = isValid(*mbr.bootSignature,0x55);

Of course... since your 'isValid' function is so simple... you don't even really need it at all. Instead you can just do this:

*mbr.bootSignatureValid = (*mbr.bootSignature == 0x55);

#4: bootSignatureValid is a pointer, but you never make it point to anything. Therefore, when you write to it on line 22, you are writing to (and corrupting) random memory. Bad bad bad bad.




From what it looks like... all you want to do is open a file... skip to offset 510, and read 2 bytes to make sure they are 0x55 and 0xAA, right?

If that's the case, this is quite simple:

#include <fstream>
#include <iostream>
using namespace std;

int main(int argc, char** argv)
{
    ifstream datafile( argv[1], ios::binary );

    datafile.seekg( 510 );  // skip to offset 510

    unsigned char bytes[2];
    datafile.read( reinterpret_cast<char*>(bytes), 2 ); // read 2 bytes to our 'bytes' array

    if( (bytes[0] == 0x55) && (bytes[1] == 0xAA) )
        cout << "yep" << endl;
    else
        cout << "nope" << endl;
}

[IWishIKnew]

The only things I would add to Disch's code is this:

#include <fstream>
#include <iostream>
using namespace std;

int main(int argc, char** argv)
{
    ifstream datafile( argv[1], ios::binary );

    if(datafile.good())  //make sure we actually have the file open
    {
        if(datafile.seekg(510).good()) //offset 510 bytes, and check the offset succeeded
        {
            unsigned char bytes[2];
            if(!datafile.read( reinterpret_cast<char*>(bytes), 2 ).fail()) // read 2 bytes to our 'bytes' array, and make sure we succeeded
            {
                if( (bytes[0] == 0x55) && (bytes[1] == 0xAA) )
                    cout << "yep" << endl;
                else
                    cout << "nope" << endl;
            }
            else cout<< "read fail"<< endl;
        }
        else cout<< "Error: could not seek to specified location!"<< endl;
    }
    else cout<< "silly, that's not somthing I can read!  ;)"<< endl;
    return 0;
}

All I added were a few checks to make sure the program doesn't segfault due to read errors. When you handle files on a level like this, you have to make sure each step succeeds, or you'll get either undefined behavior, or segfaults. It might be a lot for a beginner, but I thought it was worth mentioning.

Here is the documentation for basic_istream if you want to read up: http://en.cppreference.com/w/cpp/io/basic_istream

[Dan Feerst]

Hey everyone. Thanks for all the help. that seekg function was definately what I was looking for.
I am trying to implement the checks as well. I guess I really just need to read the ifstream docs.

I've tried to incorperate a little bit of everything but put the program back to the format I wanted. Basiclly I want a function to handle all the dirty work, and a data structure to hold data and a boolean value for validation purposes. It would be great if this function could be in pointer form, but that probably isn't possible if I close the file at the end of the function right?

If I planned to pull many chunks out of this, would it make more sense to keep the file open throughout all of the operations and use pointers?

#include <iostream>
#include <fstream>
using namespace std;
typedef unsigned int uint;
typedef unsigned char uchar;
struct mbr_t{
	uchar bootSignature[2];  
	bool bootSignatureValid;
	}mbr;

void getData( const char* filename, const uint offset, const uint bytes, uchar* storage, bool check){
	ifstream datafile ( filename , ios::binary);
    if(datafile.good()){
    	if(datafile.seekg( offset ).good()){
            if(!datafile.read( reinterpret_cast<char*>(storage), bytes ).fail()){
                if( (mbr.bootSignature[0] == 0x55) && (mbr.bootSignature[1] == 0xAA) )
                    cout << "yep" << endl;
                else
                    check = false;
            }
            else check = false;
        }
        else check = false;
	}
    else check = false;
    datafile.close();
} 

int main(int argc, char** argv){  
    
    getData( argv[1], 510, 2, mbr.bootSignature, mbr.bootSignatureValid); 
    
    
    return 0;
}

[Disch]

You have a weird conceptual problem with functions.

Typically... in the most simplistic casts.... Parameters are for input and the return value is for output. You are trying to use parameters for both input and output from a function -- and while this can be done, it should be avoided because it makes code harder to write, more error prone, and more confusing.

Your function is conceptually very simple. It has some input:

input 1) A stream
input 2) A position in that stream
input 3) Number of bytes to read from that stream (although this is questionable -- I'll get more into this later)

And it has some output:
output 1) The boot signature
output 2) Whether or not the boot signature is valid.



Now... here's the thing.... Those 2 outputs are contained in your mbr_t struct. So you really only have 1 output: an mbr_t. Since you only have 1 output, use the return value to output it:

mbr_t checkBootSignature( const char* filename, uint offset, uint bytes )
{
    mbr_t output;

    // fill 'output' with the desired data from the given filename.

    return output;  // <- return it as the output of this function
}

int main(int argc, char** argv)
{
    mbr_t mbr = checkBootSignature( argv[1], 510, 2 );  // <- check the boot signature
         // returned output is stored in our 'mbr' variable.
         //   (also note I made it local to main instead of being global because globals are
         //    yukky)
}

So you can consolidate your output there to make your function more practical. But your input is questionable as well.

For starters... your function will utterly fail unless 'bytes' is 2. Any other value will totally screw up the works. Unless you are planning for this function to do more work than it currently does, the bytes parameter doesn't really make a lot of sense.


Furthermore... as you mentioned, the function is opening/closing the file which is inefficient, especially if you are going to be reading several blocks from this file. Therefore you should probably pass the stream itself to the function by reference, rather than the filename.

So let's touch this up a bit more:

mbr_t checkBootSignature( istream& stream, uint offset )
{
    mbr_t output;

    // fill 'output' with the desired data from the given stream+offset

    return output;  // <- return it as the output of this function
}

int main(int argc, char** argv)
{
    ifstream bootfile( argv[1] ); // now main is responsible for opening/closing the file

    mbr_t mbr = checkBootSignature( bootfile, 510 );  // <- pass that file to our function

    // now you can do it repeatedly without closing/reopening the same file:
    mbr_t foobar = checkBootSignature( bootfile, 620 );
}

Lastly... I would consider removing the 'offset' parameter as well... and leave it up to the calling code to put the stream where it needs to be... but that's a judgement call.

Generally you want functions to do 1 thing. If you overreach they tend to get clunky. Here, the function is gathering information about the boot header. That's it's job. When you start having it do other things that aren't really related to that (opening/seeking through a file), then it gets weird.

[Dan Feerst]

Hey, Thanks for all the help. I'll try that out. Just to explain my actions though, my attempt was to have one rather complicated function and one data structure to pull all of the data I wanted. but honestly. I'm guessing your method is more efficient, and I already know it's less problematic. I'll try your way.

[Disch]

my attempt was to have one rather complicated function

Some advice: Split bigger jobs into smaller parts so that all your functions are small and simple. Remember that functions can call other functions, so you can have a single function that does a complex job by having it call several smaller, simpler functions.

When your code is small, straightforward, and simple... it tends to be easier to understand, easier to write, and will have fewer bugs.

[dhayden]

I suggest a completely different approach. Create a class that represents the master boot record. Read the MBR and have your class provide access to it. Something like:

class mbr_t {
public:
    char pad[510];
    char signature[2];
    bool read(const char *filename);
    bool isValidSig();
    // etc.
};

mbr_t::isValidSig()
{
    return signature[0] == 0x55 && signature[1] == 0xaa;
}

Now you only have to read the MBR once. You can add methods to access the other fields as appropriate.

[IWishIKnew]

I would not write a class at all: it is a sideways approah to a rather simple problem. Writing a class for somthing like this is overkill, but if you plan on expanding your program to add more features, then I might suggest using a class to augment your datastructure.

This, however, is a simple matter of taking information and manipulating it. Just gather that information into a data structure, and manipulate it however you like.

Having to write functions just to "access" member variables of a class is convoluted when compared to using a simple datastructure.