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
--
To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: https://lists.debian.org/5422fb45....@qo.cx
