Re: kdiff3: FTBFS on hurd-i386 (for review)

["Philipp A. Hartmann" <pah@qo.cx>]

Hi,

On 24/09/14 18:11, Lancelot SIX wrote:
> The most notable one would be to use new / delete instead of malloc /
> free [1]:
>> s = new char[sb.st_size + 1];

I would recommend to use a std::vector<char> instead, which avoids
manually deallocating the buffer.  Maybe some QT4 functions do raise
exceptions, which would lead to memory leaks then.

std::vector<char> s(sb.st_size + 1);
// use &s[0] to access the allocated buffer

>> delete[] s;

Not needed anymore then.

> Also, using cerr is more common in c++ programs, and then easier to read
> for c++ programmers.

The original code falls back to call fi.readLink() in case of a failing
readlink() call.  Your patch should do the same.

Calling exit() is bad practice in most C++ programs.  Even more so in a
GUI application, which might not even have a visible console attached
where the error message can be read.

> On Wed, Sep 24, 2014 at 03:51:12PM +0200, Svante Signell wrote:
>> - This patch has a lot of C-code in it in the C++ file.

As said above, prefer a std::vector<char> over malloc/free.

>> - The patch contains fprintf statements, should cout/no printing be used
>> instead?

Don't use fprintf and exit, just fall back to fi.readLink() as before.

>> --- a/src-QT4/fileaccess.cpp.orig	2014-07-03 13:37:37.000000000 +0200
>> +++ b/src-QT4/fileaccess.cpp		2014-09-24 14:52:37.000000000 +0200
>> @@ -235,8 +235,32 @@
>>           d()->m_linkTarget = fi.readLink();
>>  #else
>>           // Unfortunately Qt4 readLink always returns an absolute path, even if the link is relative
>> -         char s[PATH_MAX+1];
>> -         int len = readlink(QFile::encodeName(fi.absoluteFilePath()).constData(), s, PATH_MAX);
>> +	 struct stat sb;

Make sure to use the same indentation as the original source (spaces
only), instead of introducing tabs.

>> +	 char *s;
>> +	 ssize_t len;

In C++, it is common practice to declare the variables alongside with
their first use.  No need to put them at the beginning of the scope.

>> +	 const char *path = QFile::encodeName(fi.absoluteFilePath()).constData();
>> +
>> +	 if (lstat(path, &sb) == -1)
>> +	   exit(EXIT_FAILURE);

Terminating the application silently is not nice.
Also try to following the upstream formatting style.

>> +	 s = (char*)malloc(sb.st_size + 1);
>> +	 if (s == NULL) {
>> +	   fprintf(stderr, "insufficient memory\n");
>> +	   exit(EXIT_FAILURE);
>> +	 }

This is not needed when using new or std::vector.

>> +	 len = readlink(path, s, sb.st_size + 1);
>> +	 if (len == -1) {
>> +	   perror("readlink");
>> +	   exit(EXIT_FAILURE);
>> +	 }
>> +
>> +	 if (len > sb.st_size) {
>> +	   fprintf(stderr, "symlink increased in size "
>> +		   "between lstat() and readlink()\n");
>> +	   exit(EXIT_FAILURE);
>> +	 }

See above. Falling back to fi.readLink() is closer to the original code.
I would probably do it like this (untested):

    const char *path =
        QFile::encodeName(fi.absoluteFilePath()).constData();
    struct stat sb;
    bool resolved = false;
    if( lstat(path, &sb) != -1 )
    {
        std::vector<char> s(sb.st_size + 1);
        ssize_t len = readlink(path, &s[0], s.size());
        if ( len > 0 && len <= sb.st_size )
        {
            resolved = true;
            s[len] = '\0';
            d()->m_linkTarget = QFile::decodeName(&s[0]);
        }
    }
    if ( !resolved )
    {
        d()->m_linkTarget = fi.readLink();
    }

Greetings from Oldenburg,
  Philipp