1

Let's say I have a class that I don't own: DataBuffer. It provides various get member functions:

get(uint8_t *value);
get(uint16_t *value); 
...

When reading from a structure contained in this buffer, I know the order and size of fields, and I want to reduce the chance of future code changes causing an error:

struct Record 
{
    uint16_t Header;
    uint16_t Content;
}

void ReadIntoRecord(Record* r)
{
    DataBuffer buf( initialized from the network with bytes )
    buf.get(&r->Header); // Good!
    buf.get(&r->Content);
}

Then someone checks in a change to do something with the header before writing it:

    uint8_t customHeader;
    buf.get(&customHeader);  // Wrong, stopped reading after only 1 byte
    r->Header = customHeader + 1;
    buf.get(&r->Content);  // now we're reading from the wrong part of the buffer.

Is the following an acceptable way to harden the code against changes? Remember, I can't change the function names to getByte, getUShort, etc. I could inherit from DataBuffer, but that seems like overkill.

    buf.get(static_cast<uint16_t*>(&r->Header));  // compiler will catch incorrect variable type
    buf.get(static_cast<uint16_t*>(&r->Content))

Updated with not-eye-safe legacy code example:

       float dummy_float;
        uint32_t dummy32;
        uint16_t dummy16;
        uint8_t dummy8;

        uint16_t headTypeTemp;
        buf.get(static_cast<uint16_t*>(&headTypeTemp));
        m_headType = HeadType(headTypeTemp);
        buf.get(static_cast<uint8_t*>(&hid));
        buf.get(m_Name);
        buf.get(m_SerialNumber);


        float start;
        buf.get(static_cast<float*>(&start));
        float stop;
        buf.get(static_cast<float*>(&stop));


        buf.get(static_cast<float*>(&dummy_float));
        setStuffA(dummy_float);

        buf.get(static_cast<uint16_t*>(&dummy16));
        setStuffB(float(dummy16)/1000);

        buf.get(static_cast<uint8_t*>(&dummy8));    //reserved





        buf.get(static_cast<uint32_t*>(&dummy32));
        Entries().setStart( dummy32 );
        buf.get(static_cast<uint32_t*>(&dummy32));
        Entries().setStop( dummy32 );
        buf.get(static_cast<float*>(&dummy_float));
        Entries().setMoreStuff( dummy_float );

        uint32_t datalength;
        buf.get(static_cast<uint32_t*>(&datalength));

        Entries().data().setLength(datalength);

        RetVal ret = ReturnCode::SUCCESS;
        Entry* data_ptr = Entries().data().data();
        for (unsigned int i = 0; i < datalength && ret == ReturnCode::SUCCESS; i++)
        {
            ret = buf.get(static_cast<float*>(&dummy_float));
            data_ptr[i].FieldA = dummy_float;
        }

        for (unsigned int i = 0; i < datalength && ret == ReturnCode::SUCCESS; i++)
        {
            ret = buf.get(static_cast<float*>(&dummy_float));
            data_ptr[i].FieldB = dummy_float;
        }

        // Read in the normalization vector
        Util::SimpleVector<float> norm;
        buf.get(static_cast<uint32_t*>(&datalength));
        norm.setLength(datalength);
        for (unsigned int i=0; i<datalength; i++)
        {
            norm[i] = buf.getFloat();
        }

        setNormalization(norm);

        return ReturnCode::SUCCESS;
}
Cat Zimmermann
  • 1,422
  • 2
  • 21
  • 38
  • 3
    no because the same guy will change the static_cast to be uint8_t ("because it was the only way to make the compiler stop complaining"). – Kevin Aug 31 '11 at 22:46
  • 2
    Please give a reason when voting down a question. – Cat Zimmermann Aug 31 '11 at 23:13
  • AFAIK, the static cast will either result in an error, or a uint16_t. So it seems like that makes your code a little more robust..at least in terms of making sure the typing is correct. – prelic Sep 01 '11 at 02:43

3 Answers3

1

Don't use overloading. Why not have get_word and get_dword calls? The interface isn't going to be any uglier but at least the mistake is a lot harder to make.

pmr
  • 58,701
  • 10
  • 113
  • 156
1

wouldn't it be better to read the whole struct from the network? Letting the user do all the socket operations seems like a bad idea to me (not encapsulated). Encapsulate the stuff you want to send on the network to operate on file descriptors instead of letting the user put raw buffer data to the file descriptors.

I can imagine something like 

void readHeader(int filedes, struct Record * Header);

so you can do something like this

struct Record 
{
  uint16_t Header;
  uint16_t Content;
  uint16_t getHeader() const { return Header; }
  uint16_t getContent() const  { return Content; }  
};

/* socket stuff to get filedes */
struct Record x;
readHeader(fd, &x);
x.getContent();
Alexander Oh
  • 24,223
  • 14
  • 73
  • 76
  • Yes, that would be better, but my example was too contrived; the struct does not exist in the legacy code I'm dealing with. There are variable length fields, for example. – Cat Zimmermann Sep 01 '11 at 16:43
  • in how far is it worse? could you improve the example? is the parsing logic separated from the other logic could you use inheritance to reflect the buffer parsing logic? how much do you want to add? – Alexander Oh Sep 01 '11 at 21:20
  • Added a quick paste of what I'm diving into. – Cat Zimmermann Sep 01 '11 at 21:44
  • looking at THAT code I don't know what to answer, it looks like some kind of weird parser. You could consider using a parser generator when you know the grammar. – Alexander Oh Sep 01 '11 at 22:35
-1

You can't read from buffer with type safety unless the buffer contains information about the content. One simple method is to add length to each structure and check that at least the data being read is still the sane length. You could also use XML or ASN.1 or something similar where type information is provided. Of course I'm assuming that you also write to that buffer.

Kalle
  • 11
  • I'm not asking about how to gain type safety through extending the protocol. I'm asking if those two lines of code are considered a good way to harden the code. – Cat Zimmermann Aug 31 '11 at 23:12