Hi
I did not have a look to the entire original file, just the patch.
I would recommend to use C++ features instead of C ones in C++ programs
(unless C ones are used all over the file, being coherent is important
also).
The most notable one would be to use new / delete instead of malloc /
free [1]:
> s = new char[sb.st_size + 1];
> ...
> delete[] s;
Also, using cerr is more common in c++ programs, and then easier to read
for c++ programmers.
> std:cerr << "insufficient memory" << std::endl
> ....
I think that if the compiler is happy even if you did not include
stdio.h (for standard features, you could use #include <cstdio> offered
by standard c++ to access c functions) it is because one included file
should include it also at some point. So no real worry here.
Any way, what you do is valid, (even it would be preferable to use new /
delete), it is nearly just a matter of style to use the cpp way.
BR
Lancelot
[1] http://stackoverflow.com/questions/184537/in-what-cases-do-i-use-malloc-vs-new
On Wed, Sep 24, 2014 at 03:51:12PM +0200, Svante Signell wrote:
> Source: kdiff3
> Version: 0.9.98-1
> Severity: important
> Tags: patch
> User: debian-hurd@lists.debian.org
> Usertags: hurd
>
> Hi, kdiff3 does not build on GNU/Hurd since PATH_MAX is not defined. It
> did build before, and the latest Hurd version is 0.9.97-3. Instead of
> using realpath() with a fixed size buffer length this patch use the
> lstat() call to find out the necessary string length, dynamically
> allocate it, and free it when no longer needed.
>
> Thanks!
>
> Questions:
>
> - This patch has a lot of C-code in it in the C++ file.
>
> - The patch contains fprintf statements, should cout/no printing be used
> instead?
>
> - No complaints from the compiler, even if <stdio.h> is not included,
> etc?
>
> - Return values of the function are not used in the original code, the
> return value is void. Is that needed here? Better ways to exit the
> function on error?
> --- 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;
> + char *s;
> + ssize_t len;
> + const char *path = QFile::encodeName(fi.absoluteFilePath()).constData();
> +
> + if (lstat(path, &sb) == -1)
> + exit(EXIT_FAILURE);
> +
> + s = (char*)malloc(sb.st_size + 1);
> + if (s == NULL) {
> + fprintf(stderr, "insufficient memory\n");
> + exit(EXIT_FAILURE);
> + }
> +
> + 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);
> + }
> +
> if ( len>0 )
> {
> s[len] = '\0';
> @@ -246,6 +270,7 @@
> {
> d()->m_linkTarget = fi.readLink();
> }
> + free(s);
> #endif
> }
> d()->m_bLocal = true;
Attachment:
pgpVNoAWAkykd.pgp
Description: PGP signature