Re: Need advice if changes to file_5.04-6 enabling build on GNU/Hurd are OK?
On Thu, 2011-02-10 at 09:16 +0100, olafBuddenhagen@gmx.net wrote:
> Hi,
>
> On Thu, Feb 10, 2011 at 12:18:59AM +0100, Svante Signell wrote:
>
> > The pattern I used for the patch was
> > +#ifdef __GLIBC__
> > + char * subfn;
> > +#else
> > + char subfn[MAXPATHLEN];
> > +#endif
> > + char **filearr = NULL, *mfn;
>
> Hope you also have a free(subfn) somewhere in there :-)
Yes I have, see the attached patch.
> > Should I use #ifdef HAVE_ASPRINTF and #ifdef HAVE_GETLINE instead?
>
> Well, it's a largely a matter of taste. IMHO it would be more elegant to
> use asprintf() whenever available -- but depending on the upstream
> maintainer's belief system, it *might* be preferable to only use it if
> it's strictly necessary, i.e. if MAXPATHLEN doesn't exist...
>
> (Ignoring the fact that in theory at least, MAXPATHLEN could be a large
> value, making the static approach unsuitable even when MAXPATHLEN
> exists.)
>
> I'd suggest proposing a patch with whatever you personally prefer, and
> see how upstream reacts.
Since upstream is already using asprintf (and vasprinf) in apprentice.c
(and funcs.c) I skip the check for asprintf. In fact upstream includes
c-code for both asprintf and vasprintf! configure.ac:
AC_REPLACE_FUNCS(getopt_long asprintf vasprintf strlcpy strlcat)
I keep the #if defined(__GLIBC__) || defined(HAVE_GETLINE) check in
file.c in case getline is not defined.
Looks like the coding style is varying: two more files are using
snprintf, cdf.c use malloc and softmagic.c use a fixed buffer size.
I'm sending this patch in a Debian bug. Should I also tag it with
upstream?
diff -ur file-5.04.orig/configure file-5.04/configure
--- file-5.04.orig/configure 2011-02-10 10:38:10.000000000 +0100
+++ file-5.04/configure 2010-01-22 22:09:26.000000000 +0100
@@ -23588,7 +23588,7 @@
-for ac_func in mmap strerror strndup strtoul mbrtowc mkstemp utimes utime wcwidth strtof getline
+for ac_func in mmap strerror strndup strtoul mbrtowc mkstemp utimes utime wcwidth strtof
do
as_ac_var=`echo "ac_cv_func_$ac_func" | $as_tr_sh`
{ echo "$as_me:$LINENO: checking for $ac_func" >&5
diff -ur file-5.04.orig/src/apprentice.c file-5.04/src/apprentice.c
--- file-5.04.orig/src/apprentice.c 2009-10-19 15:10:20.000000000 +0200
+++ file-5.04/src/apprentice.c 2011-02-10 10:43:08.000000000 +0100
@@ -72,10 +72,6 @@
#define MAP_FILE 0
#endif
-#ifndef MAXPATHLEN
-#define MAXPATHLEN 1024
-#endif
-
struct magic_entry {
struct magic *mp;
uint32_t cont_count;
@@ -708,7 +704,7 @@
struct magic_entry *marray;
uint32_t marraycount, i, mentrycount = 0, starttest;
size_t slen, files = 0, maxfiles = 0;
- char subfn[MAXPATHLEN], **filearr = NULL, *mfn;
+ char * subfn, **filearr = NULL, *mfn;
struct stat st;
DIR *dir;
struct dirent *d;
@@ -735,8 +731,7 @@
goto out;
}
while ((d = readdir(dir)) != NULL) {
- (void)snprintf(subfn, sizeof(subfn), "%s/%s",
- fn, d->d_name);
+ (void)asprintf (&subfn, "%s/%s", fn, d->d_name);
if (stat(subfn, &st) == -1 || !S_ISREG(st.st_mode))
continue;
if ((mfn = strdup(subfn)) == NULL) {
@@ -757,6 +752,7 @@
}
filearr[files++] = mfn;
}
+ free (subfn);
closedir(dir);
qsort(filearr, files, sizeof(*filearr), cmpstrp);
for (i = 0; i < files; i++) {
diff -ur file-5.04.orig/src/file.c file-5.04/src/file.c
--- file-5.04.orig/src/file.c 2009-12-14 18:08:02.000000000 +0100
+++ file-5.04/src/file.c 2011-02-10 10:11:28.000000000 +0100
@@ -85,10 +85,6 @@
" %s -C [-m magicfiles]\n" \
" %s [--help]\n"
-#ifndef MAXPATHLEN
-#define MAXPATHLEN 1024
-#endif
-
private int /* Global command-line options */
bflag = 0, /* brief output format */
nopad = 0, /* Don't pad output */
@@ -358,7 +354,13 @@
private int
unwrap(struct magic_set *ms, const char *fn)
{
- char buf[MAXPATHLEN];
+#if defined(__GLIBC__) || defined(HAVE_GETLINE)
+ static char * buf;
+ size_t dummy = 0;
+#else
+ char buf[MAXPATHLEN];
+#endif
+
FILE *f;
int wid = 0, cwid;
int e = 0;
@@ -372,8 +374,11 @@
progname, fn, strerror(errno));
return 1;
}
-
+#if defined(__GLIBC__) || defined(HAVE_GETLINE)
+ while (getline(&buf, &dummy, f) != -1) {
+#else
while (fgets(buf, sizeof(buf), f) != NULL) {
+#endif
buf[strcspn(buf, "\n")] = '\0';
cwid = file_mbswidth(buf);
if (cwid > wid)
@@ -383,7 +388,11 @@
rewind(f);
}
+#if defined(__GLIBC__) || defined(HAVE_GETLINE)
+ while (getline(&buf, &dummy, f) != -1) {
+#else
while (fgets(buf, sizeof(buf), f) != NULL) {
+#endif
buf[strcspn(buf, "\n")] = '\0';
e |= process(ms, buf, wid);
if(nobuffer)
@@ -391,6 +400,9 @@
}
(void)fclose(f);
+#if defined(__GLIBC__) || defined(HAVE_GETLINE)
+ free(buf);
+#endif
return e;
}
diff -ur file-5.04.orig/src/file.h file-5.04/src/file.h
--- file-5.04.orig/src/file.h 2010-01-20 02:32:39.000000000 +0100
+++ file-5.04/src/file.h 2011-02-10 10:24:58.000000000 +0100
@@ -51,7 +51,13 @@
#endif
#include <regex.h>
#include <sys/types.h>
+
+#ifdef __GNU__
+#include <features.h>
+#else
#include <sys/param.h>
+#endif
+
/* Do this here and now, because struct stat gets re-defined on solaris */
#include <sys/stat.h>
#include <stdarg.h>
diff -ur file-5.04.orig/src/magic.c file-5.04/src/magic.c
--- file-5.04.orig/src/magic.c 2009-09-14 19:50:38.000000000 +0200
+++ file-5.04/src/magic.c 2011-02-10 10:50:59.000000000 +0100
@@ -85,22 +85,20 @@
get_default_magic(void)
{
static const char hmagic[] = "/.magic";
- static char default_magic[2 * MAXPATHLEN + 2];
+ static char * default_magic;
char *home;
- char hmagicpath[MAXPATHLEN + 1];
+ char * hmagicpath;
if ((home = getenv("HOME")) == NULL)
return MAGIC;
- (void)snprintf(hmagicpath, sizeof(hmagicpath), "%s%s", home, hmagic);
-
+ (void)asprintf(&hmagicpath, "%s%s", home, hmagic);
if (access(hmagicpath, R_OK) == -1)
return MAGIC;
-
- (void)snprintf(default_magic, sizeof(default_magic), "%s:%s",
- hmagicpath, MAGIC);
-
+ (void)asprintf(&default_magic, "%s:%s", hmagicpath, MAGIC);
return default_magic;
+ free (hmagicpath);
+ free (default_magic);
}
public const char *
Reply to: