CODEWITHSUNDEEP
c++windowsqtqt5qtserialport
smsware
smsware

Reputation: 479

QtSerialPort has unavailable data, segmentation fault under Windows 8 only

const QSerialPortInfo* serialPortInfo = nullptr;

bool PortManager::setPort(QString portName) {
    const QList<QSerialPortInfo> infoList = QSerialPortInfo::availablePorts();
    for (const QSerialPortInfo portInfo : infoList) {
        if (portInfo.portName() == portName && serialPortInfo != &portInfo) {
            serialPortInfo = &portInfo;
        }
    }
    if (serialPortInfo != nullptr) {
        if (portName != "" && serialPortInfo->isValid()) { //segmentation fault
            if (serialPort->isOpen()) {
                serialPort->close();
            }
            serialPort = new QSerialPort(*serialPortInfo, this);
            if (serialPort->open(QIODevice::ReadWrite)) {
                if (serialPort->clear()) {
                    if (serialPort->setBaudRate(QSerialPort::Baud38400, QSerialPort::AllDirections)
                            && serialPort->setFlowControl(QSerialPort::NoFlowControl)
                            && serialPort->setParity(QSerialPort::NoParity)) {
                        isPortSet = true;
                    }
                    .
                    .
                    .

This is my code which works both on Linux and Windows 7. Now I'm testing it on Windows 8 and I'm getting segmentation fault on this->serialPortInfo->isValid() (and any other function of serialPortInfo). All the data of any specific QSerialPortInfo object is "unavailable" (as debugger states it) which seems to me that I don't have some priviliges to use them. On Linux, I had to be a member of uupc (if I recall correctly) group to not have such errors but on Windows 7 I didn't have to do a thing. I run Qt Creator "as Administrator" but it doesn't help; maybe I have to somehow tell it to run qmake as Administrator too? But it's just my guess, maybe the reason is different...

Upvotes: 3

Views: 1172

Answers (2)

L&#225;szl&#243; Papp
L&#225;szl&#243; Papp

Reputation: 53165

This is a typical case of why Qt encourages you to use const reference in for loops when you do not plan to change the content. See this line:

for (const QSerialPortInfo portInfo : infoList) {

You ought to be writing this to make it work:

for (const QSerialPortInfo &portInfo : infoList) {
//                         ^

The reason is very simple, you are creating a temporary copy which get destroyed when going out-of-scope rather than using the original list item which will outlive the loop since the original list is outside.

You must be happy that it works at all on Linux and Windows 7. I am very surprised about that and even if it does, it might blow up at your customer at any certain moment.

That being said, your concept seems to be wrong overall. You query all the items to find one dedicated port. It would be much clearer to just construct your QSerialPortInfo instance in the first place like this:

serialPortInfo = new QSerialPortInfo(portName);

Moreover, you really ought to be using a stack object rather than heap. Info classes like that are not meant to be allocated on the heap, especially without smart pointer management.

It is also needless to say that making a global variable when they are evil is bad, especially in this case, where you could avoid it by putting that into the method itself.

You could also then get rid of the following line if you stop using a pointer:

if (serialPortInfo != nullptr) {

Not to mention, in a Qt application, you should be using Q_NULLPTR anyway, since that will also work without C++11 support, and the same reason goes to your original for loop. I would use foreach from Qt, but then again, I think the overall concept is wrong.

It seems that you do not even need a QSerialPortInfo instance since all you are using is just the name which you could pass directly to the QSerialPort object. Hence, you could get rid of even the QSerialPortInfo object.

Therefore, just drop the for loop and QSerialPortInfo instance altogether and use QSerialPort directly within the method is my suggestion.

Upvotes: 1

Some programmer dude
Some programmer dude

Reputation: 409196

The problem is this loop:

for (const QSerialPortInfo portInfo : infoList) {
    if (portInfo.portName() == portName && serialPortInfo != &portInfo) {
        serialPortInfo = &portInfo;
    }
}

The variable portInfo has its scope inside the loop only, and for the current iteration only. Once the loop iterates, that variable is destructed. Using a pointer to a destructed object is undefined behavior, and the probable cause of your crashes.

My advice for you on how to stop it is to not use a pointer. Instead copy the structure.

Upvotes: 3

Related Questions