configure.in | 2 +
src/fcstat.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 55 insertions(+), 8 deletions(-)
New commits:
commit 4a741e9a0ab8dbaa0c377fbfed41547645ac79af
Author: Akira TAGOH <akira at tagoh.org>
Date: Wed May 30 18:21:57 2012 +0900
Fix the build fail on Solaris
It''s introduced by 0ac6c98294d666762960824d39329459b22b48b7.
Use lstat() and S_ISDIR() to check if it''s the directory or not
if there are no d_type in struct dirent.
diff --git a/configure.in b/configure.in
index b2174d9..f455cb5 100644
--- a/configure.in
+++ b/configure.in
@@ -154,6 +154,8 @@ if test "x$ac_cv_func_fstatfs" = "xyes";
then
#include <sys/mount.h>
#endif])
fi
+AC_CHECK_MEMBERS([struct dirent.d_type],,,
+ [#include <dirent.h>])
#
# regex
#
diff --git a/src/fcstat.c b/src/fcstat.c
index c2d9fe9..c67c434 100644
--- a/src/fcstat.c
+++ b/src/fcstat.c
@@ -163,7 +163,11 @@ Adler32Finish (struct Adler32 *ctx)
static FcBool
FcDirChecksumScandirFilter(const struct dirent *entry)
{
+#ifdef HAVE_STRUCT_DIRENT_D_TYPE
return entry->d_type != DT_DIR;
+#else
+ return FcFalse;
+#endif
}
static int
@@ -177,25 +181,66 @@ FcDirChecksum (const FcChar8 *dir, time_t *checksum)
{
struct Adler32 ctx;
struct dirent **files;
- int n;
+ int n, ret = 0;
+#ifndef HAVE_STRUCT_DIRENT_D_TYPE
+ size_t len = strlen ((const char *)dir);
+#endif
Adler32Init (&ctx);
n = scandir ((const char *)dir, &files,
- &FcDirChecksumScandirFilter,
- &FcDirChecksumScandirSorter);
+ &FcDirChecksumScandirFilter,
+ &FcDirChecksumScandirSorter);
if (n == -1)
- return -1;
+ return -1;
while (n--)
{
- Adler32Update (&ctx, files[n]->d_name,
strlen(files[n]->d_name) + 1);
- Adler32Update (&ctx, (char *)&files[n]->d_type,
sizeof(files[n]->d_type));
- free(files[n]);
+ size_t dlen = strlen (files[n]->d_name);
+ int dtype;
+
+#ifdef HAVE_STRUCT_DIRENT_D_TYPE
+ dtype = files[n]->d_type;
+#else
+ struct stat statb;
+ char *f;
+
+ f = malloc (len + 1 + dlen + 1);
+ if (!f)
+ {
+ ret = -1;
+ goto bail;
+ }
+ memcpy (f, dir, len);
+ f[len] = FC_DIR_SEPARATOR;
+ memcpy (&f[len + 1], files[n]->d_name, dlen);
+ f[len + 1 + dlen] = 0;
+ if (lstat (f, &statb) < 0)
+ {
+ ret = -1;
+ goto bail;
+ }
+ if (S_ISDIR (statb.st_mode))
+ goto bail;
+
+ dtype = statb.st_mode;
+#endif
+ Adler32Update (&ctx, files[n]->d_name, dlen + 1);
+ Adler32Update (&ctx, (char *)&dtype, sizeof (int));
+
+#ifndef HAVE_STRUCT_DIRENT_D_TYPE
+ bail:
+ if (f)
+ free (f);
+#endif
+ free (files[n]);
}
- free(files);
+ free (files);
+ if (ret == -1)
+ return -1;
*checksum = Adler32Finish (&ctx);
+
return 0;
}
On 05/30/12 02:34 AM, tagoh at kemper.freedesktop.org wrote:> > Fix the build fail on Solaris > > It''s introduced by 0ac6c98294d666762960824d39329459b22b48b7. > Use lstat() and S_ISDIR() to check if it''s the directory or not > if there are no d_type in struct dirent. >Sorry, I haven''t been paying as much attention to this as I should, but I did notice this thread and wonder about this fix.> @@ -163,7 +163,11 @@ Adler32Finish (struct Adler32 *ctx) > static FcBool > FcDirChecksumScandirFilter(const struct dirent *entry) > { > +#ifdef HAVE_STRUCT_DIRENT_D_TYPE > return entry->d_type != DT_DIR; > +#else > + return FcFalse; > +#endif > }Isn''t this backwards? fontconfig/fontconfig.h has #define FcFalse 0, but at least the Solaris scandir man page says The select argument is a pointer to a routine that is called with a pointer to a directory entry and returns a non-zero value if the direc- tory entry is included in the array. which means this implementation will discard all entries and just waste a bunch of CPU doing it. It also says If this pointer is NULL, then all the directory entries are included. which suggests a simpler and more efficient fix of: --- a/src/fcstat.c +++ b/src/fcstat.c @@ -159,16 +159,14 @@ Adler32Finish (struct Adler32 *ctx) return ctx->a + (ctx->b << 16); } +#ifdef HAVE_STRUCT_DIRENT_D_TYPE /* dirent.d_type can be relied upon on FAT filesystem */ static FcBool FcDirChecksumScandirFilter(const struct dirent *entry) { -#ifdef HAVE_STRUCT_DIRENT_D_TYPE return entry->d_type != DT_DIR; -#else - return FcFalse; -#endif } +#endif static int FcDirChecksumScandirSorter(const struct dirent **lhs, const struct dirent **rhs @@ -189,7 +187,11 @@ FcDirChecksum (const FcChar8 *dir, time_t *checksum) Adler32Init (&ctx); n = scandir ((const char *)dir, &files, +#ifdef HAVE_STRUCT_DIRENT_D_TYPE &FcDirChecksumScandirFilter, +#else + NULL /* filter below instead of in scandir */ +#endif &FcDirChecksumScandirSorter); if (n == -1) return -1; It also looks like you could save a whole bunch of malloc calls by either using a fixed size buffer of PATH_MAX, or saving the f pointer and realloc''ing it to be larger if needed, instead of malloc and free every time, such as: @@ -184,6 +182,8 @@ FcDirChecksum (const FcChar8 *dir, time_t *checksum) int n, ret = 0; #ifndef HAVE_STRUCT_DIRENT_D_TYPE size_t len = strlen ((const char *)dir); + size_t flen = 0; + char *f = NULL; #endif Adler32Init (&ctx); @@ -203,14 +203,18 @@ FcDirChecksum (const FcChar8 *dir, time_t *checksum) dtype = files[n]->d_type; #else struct stat statb; - char *f; - - f = malloc (len + 1 + dlen + 1); - if (!f) - { - ret = -1; - goto bail; - } + int total_len = len + 1 + dlen + 1; + + if (total_len > flen) { + char *new_f = realloc(f, total_len); + if (!new_f) + { + free(f); + ret = -1; + goto bail; + } + f = new_f; + } memcpy (f, dir, len); f[len] = FC_DIR_SEPARATOR; memcpy (&f[len + 1], files[n]->d_name, dlen); @@ -230,11 +234,12 @@ FcDirChecksum (const FcChar8 *dir, time_t *checksum) #ifndef HAVE_STRUCT_DIRENT_D_TYPE bail: - if (f) - free (f); #endif free (files[n]); } +#ifndef HAVE_STRUCT_DIRENT_D_TYPE + free (f); +#endif free (files); if (ret == -1) return -1; Both of those are just suggestions which I have not tested (or even tried to compile). -- -Alan Coopersmith- alan.coopersmith at oracle.com Oracle Solaris Engineering - http://blogs.oracle.com/alanc
(Don''t you hate when you spot the mistakes in your e-mail as soon as it arrives back in your inbox? I did mention not testing either suggestion, right?) On 05/30/12 07:59 AM, Alan Coopersmith wrote:> + if (total_len > flen) { > + char *new_f = realloc(f, total_len); > + if (!new_f) > + { > + free(f);Actually, since we continue looping, probably don''t want to free(f), so that we can just use it the next time through - if you do free it, then f needs to be set to NULL to avoid double free, and flen needs to be reset to 0 so we start the allocation anew on the next loop.> + ret = -1; > + goto bail; > + } > + f = new_f;There should of course also be: flen = total_len; here.> + }-- -Alan Coopersmith- alan.coopersmith at oracle.com Oracle Solaris Engineering - http://blogs.oracle.com/alanc
On Wed, May 30, 2012 at 11:59 PM, Alan Coopersmith <alan.coopersmith at oracle.com> wrote:> Sorry, I haven''t been paying as much attention to this as I should, but > I did notice this thread and wonder about this fix.Please see git log. that should explains all what you want to know.> Isn''t this backwards? ? fontconfig/fontconfig.h has #define FcFalse 0, but at > least the Solaris scandir man page saysDoh. right. just a typo.> ? ? If ?this ?pointer ?is NULL, then all the directory entries are included. > > which suggests a simpler and more efficient fix of:Good point.> It also looks like you could save a whole bunch of malloc calls by either using > a fixed size buffer of PATH_MAX, or saving the f pointer and realloc''ing it to > be larger if needed, instead of malloc and free every time, such as:True. that makes sense.> Both of those are just suggestions which I have not tested (or even tried to > compile).Thanks for catching this up and kindly your suggestion :)> > -- > ? ? ? ?-Alan Coopersmith- ? ? ? ? ? ? ?alan.coopersmith at oracle.com > ? ? ? ? Oracle Solaris Engineering - http://blogs.oracle.com/alanc-- Akira TAGOH
On Thu, May 31, 2012 at 12:19 AM, Alan Coopersmith <alan.coopersmith at oracle.com> wrote:> (Don''t you hate when you spot the mistakes in your e-mail as soon as it > ?arrives back in your inbox? ? I did mention not testing either suggestion, > ?right?)any suggestions and pointing my mistake out is always welcome :)> Actually, since we continue looping, probably don''t want to free(f), > so that we can just use it the next time through - if you do free it, > then f needs to be set to NULL to avoid double free, and flen needs > to be reset to 0 so we start the allocation anew on the next loop.Maybe just not using malloc would be easier? anyway I''ll fix them soon. -- Akira TAGOH