4
\$\begingroup\$

I'm trying to learn C++ and I would interested in how I could write better code. The code should be independent of the architecture it is running on. What I want to do is writing some binary data to /dev/rfkill so that I can change some settings. The target is an embedded system so the program does run with root privileges and the program is using the Qt framework.

This is the code I currently have and which does what I want:

    rfkill_event event;
    event.type = RFKILL_TYPE_ALL;
    event.op = RFKILL_OP_CHANGE_ALL;
    event.soft = RFKILL_ACTION_UNBLOCK;

    QFile file("/dev/rfkill");
    if (!file.open(QIODevice::WriteOnly))
        quit();

    QDataStream out(&file);
    if (-1 == out.writeRawData((char*)(&(event.idx)), sizeof(event.idx))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.type)), sizeof(event.type))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.op)), sizeof(event.op))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.soft)), sizeof(event.soft))) {
        file.close();
        quit();
    }
    if (-1 == out.writeRawData((char*)(&(event.hard)), sizeof(event.hard))) {
        file.close();
        quit();
    }
    file.close();
    quit();

and the struct looks like this:

struct rfkill_event {
    __u32 idx;
    __u8  type;
    __u8  op;
    __u8  soft, hard;
};

The code is taken and adapted from the rfkill program.

I've removed __attribute__((packed)) from the struct because of the pitfalls mentioned here.

I'm using the writeRawData function instead of the << operator to write the data. The reason for this is that this code should work on big and little endian machines.

Are my assumptions correct and do you have any suggestions on how I could improve this code (besides not having any error handling)?

\$\endgroup\$
6
\$\begingroup\$

First I would like to say that I'm not an expert on rfkill; so my comments are more general and may be slightly off for this specific case.

Despite the missing error handling (which you mention) I see only two problems. Not primarily with your code, but with your assumptions:

  • You are correct to avoid __attribute__((packed)), but the reasons are IMHO not correct. The main reasons in your linked document are unaligned access, data size and performance. None of this is really a showstopper for your code, because you are breaking the data up into its primitive types. More significant is IMHO the simple fact that __attribute__((packed)) is not portable. (Unless you remain on GCC only. On a microsoft compiler you would write #pragma pack(1))

  • I think you are wrong using the writeRawData() function. Because your code should handle both, big endian and little endian, you would want to use the streaming << operator. I interpret the documentation like this: writeRawData() will always write in the machine byte order - big, or little endian - whatever the machine is. The streaming operators on the other hand will do a conversion, when necessary, to whatever endianness you have selected with setByteOrder().

    There is another advantage using the streaming operators, you do not need to do ugly type casting. Instead of this:

    out.writeRawData((char*)(&(event.idx)), sizeof(event.idx))
    

    you would simply write:

    out << event.idx;
    

Update: Error handling

I disagree with user1118321. I don't think that the QDataStream throws if it cannot write. I did not find anything about it in the documentation. (but the Qt documentation is known for having white spots) To handle write errors you can use the status() function. Or perhaps the functions from the underlying QIODevice.

out.resetStatus();
out << event.idx;
if(out.status() != QDataStream::Ok)
{
    // error handling
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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