Fixed segfault && data comunication bugs

[Danysan1]

Fixed an unchecked array access that caused a segfault of the daemon when the app was launched, in rpdthread.cpp at line 84. This solves the issue #2 . Fixed a QString -> char[] conversion error that prevented the daemon from sending data to the main radeon-profile app, in rpdthread.cpp at line 129. Improved the logging system: now qDebug messages will only be shown when the daemon is compiled with the debug flag, while qWarning, qCritical and qFatal will behave the same way as before. Moved the signal types into rpdthread.h as constants (it's more clear now). Added .gitignore file, useful for git.

[marazmista]

Nice work. I have one problem. When I compile it in Debug mode, everything work fine (with radeon-profile compiled from https://github.com/marazmista/radeon-profile/commit/b6b50c45eca34e435dead42b0757a5f7cf301b2f which is latest master). But in Release, nothing get into shared memory. This is exact problem that I had when I tried to fix #2. After increase this array check (in rpdthread.cpp, line 84, if (s.count() > 2) to if (s.count() > 3)) and compile everything is gone. Does it work fine for you when you compile Release?

[Danysan1]

Ok i tried launching the daemon in release mode and the first time I open radeon-profile everything works fine, but if I close and reopen it (radeon-profile), data becomes unavailable; So i tried to improve the debug/warning system of the daemon (and while doing it I fixed a problem with the timer) EDIT: Ok I found an error I made in the last commit... fixing it...

[Danysan1]

Now here it works fine on both debug and release, with auto-update on and off, on the AUR version of radeon-profile. PS: Tomorrow I'll also take a look at the the comments to my other pull request, in radeon-profile :)

[Danysan1]

Looks like something is still wrong with the release version... Most of the times it works fine, but sometimes it randomly it starts losing some 'read cycles'. It's really weird, since it happens only sometimes, and only with the release version. I captured a video of it: https://youtu.be/GrFI362LAnI I have tried to figure out what could be the cause but the only thing that comes to my mind is a lock/unlock conflict when both are trying to access the shared memory, but that makes no sense, since the second that arrives should just wait for the memory to unlock... Regarding your proposal to change if (s.count() > 2) to if (s.count() > 3)), it would exclude the 3rd part from being parsed. For instance, the signal 0#/sys/kernel/debug/dri/0/radeon_pm_info#4#1 which means CONFIGURE with path /sys/kernel/debug/dri/0/radeon_pm_info, then TIMER_ON with period 1 would instead be interpreted as CONFIGURE with path /sys/kernel/debug/dri/0/radeon_pm_info, then READ_CLOCKS since the #4 would be ignored being the third element.

[marazmista]

Yes, that happened before too, for example after profile change. As far as I know lock() doesn't wait for shared memory to be available. It just tries at given moment, and returns false when it couldn't lock. Solution could be:

while (!sharedMem.lock()) {
    QThread::usleep(10);
}

// read clocks etc...
sharedMem.unlock()

but I don't know if situation is that critical to bother.

The s.count() > 3) was just my quick solution, nothing important now, since you've rewritten this part.

[Danysan1]

On the official documentation it says: If another process has locked the segment, this function blocks until the lock is released. Then it acquires the lock and returns true. so if(!sharedMem.lock()) should correct

[marazmista]

My bad, documentation clears the confusion.

[Danysan1]

I just pushed a commit here (https://github.com/Danysan1/radeon-profile-daemon/commit/e29c062a67bc0dfc52ed16e591875ef2e805fb05) and another in radeon-profile (https://github.com/Danysan1/radeon-profile/commit/da280868ac8758cc2af3fdf0fe2c5d3a974ddb20). I've updated the communication (fixed the way the daemon parsed the instructions losing some of them, the problem I talked about here https://github.com/marazmista/radeon-profile/pull/14#issuecomment-172564942) and done some other improvements to the shared memory management (the details are in the commits). On my computer it works fine.

[marazmista]

Please, try to improve readability of your code. It is very hard going through all those nested ifs. For example, I modified this method:

void rpdThread::setNewValue(const QString &filePath, const QString &newValue) {
    // limit potetntial vulnerability of writing files as root system wide
    if (filePath.startsWith("/sys/class/drm/")){
        QFileInfo fileInfo(filePath);
        if (fileInfo.exists()){ // The indicated path exists
            if(fileInfo.isFile()){ // The path is a file, not a directory
                QFile file(filePath);
                if(file.open(QIODevice::WriteOnly | QIODevice::Text)){ // If the file opens successfully

                    qDebug() << newValue << " will be written into " << filePath;
                    QTextStream stream(&file);
                    stream << newValue + "\n";
                    if(!file.flush()) // If writing down the changes fails
                        qWarning() << "Failed writing in " << filePath;
                    file.close();

                } else // Failed to open the file
                    qWarning() << "Failed to open " << filePath;
            } else // The path is a directory
                qWarning() << "Indicated path to be set is not a valid file: " << filePath;
        } else // The path does not exist
            qWarning() << "Indicated path to be set does not exist: " << filePath;
    } else // The path is not in the whitelisted directories
        qWarning() << "Invalid path to be set: " << filePath;
}

to this:

void rpdThread::setNewValue(const QString &filePath, const QString &newValue) {
    // limit potetntial vulnerability of writing files as root system wide
    if (!filePath.startsWith("/sys/class/drm/")) {
        qWarning() << "Invalid path to be set: " << filePath;
        return;
    }

    QFile file(filePath);
    if (!file.exists()) {
        qWarning() << "Indicated path to be set does not exist: " << filePath;
        return;
    }

    if (!file.open(QIODevice::WriteOnly | QIODevice::Text)) {
        qWarning() << "Failed to open " << filePath;
        return;
    }

    qDebug() << newValue << " will be written into " << filePath;
    QTextStream stream(&file);
    stream << newValue + "\n";
    if (!file.flush()) // If writing down the changes fails
        qWarning() << "Failed writing in " << filePath;

    file.close();
}

Would you agree that latter looks simpler and it is easier to catch which if produces which debug message? I'm going to test a little more both pull request and merge at the weekend.

[Danysan1]

Perfect :+1:

[Danysan1]

Do you want me to apply these changes in a commit?

[marazmista]

Copying radeon_pm_info from debug into /tmp using QFile::copy doesn't work correctly. Copied file is empty. Can you confirm?

[Danysan1]

I can confirm. That's really strange...

[marazmista]

Ok, I will fix this in master. Thanks for contribution, good work :)