Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-16 Thread Svante Signell 
On Sat, 2011-04-16 at 02:42 +0200, Guillem Jover wrote:
> Hi!
> 
> On Tue, 2011-04-12 at 21:46:09 +0200, Svante Signell wrote:
> > Final version??
> 
> > diff -ur tofrodos-1.7.8.debian.1/src/tofrodos.c 
> > tofrodos-1.7.8.debian.1.new/src/tofrodos.c
> > --- tofrodos-1.7.8.debian.1/src/tofrodos.c  2011-04-12 18:21:50.0 
> > +0200
> > +++ tofrodos-1.7.8.debian.1/src/tofrodos.c.new  2011-04-12 
> > 21:24:27.0 +0200
> > @@ -14,7 +14,9 @@
> >  #include /* EXIT_SUCCESS, mkstemp() in some systems */
> >  #include /* strrchr(), strlen(), strcpy(), strcat() */
> >  #include   /* stat() */
> > -
> > +#ifdef __GNU__
> > +#include 
> > +#endif

> Why are you including limits.h, it does not seem to be needed.

Yes you are right, it does not seem to be needed. Don't remember why I
included it.

> So, while doing realpath() would seem to be the correct thing to do
> everywhere, as the initial symlink might point to another symlink,
> this changes behaviour only for Hurd, which I think it's a bad idea.
> But using realpath(foo, NULL) is not really portable and would need to
> be checked at build time if it was to be used everywhere.

OK! The reason for my proposed changes was to make only Hurd-specific
changes, since people seem to be very anxious about changes touching
other architectures. But if you say we could do that, no problem.

> So what about the attached patch instead (which I had around from some
> time ago when I took a look at this, although forgot it in my kvm :/,
> beware only build tested).

Your patch seems to be a better solution than mine. It will be up to the
DM to decide then.

> It also changes the kFreeBSD patch because usually __GLIBC__ is not
> predefined and it seems confusing to have it in there. (Ideally the
> build system would switch to detect features at build time, instead of
> hard-coding system support.)

Yes, this would be a very good goal indeed. Has build-time feature
detection been implemented somewhere?






-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-15 Thread Guillem Jover 
Hi!

On Tue, 2011-04-12 at 21:46:09 +0200, Svante Signell wrote:
> Final version??

> diff -ur tofrodos-1.7.8.debian.1/src/tofrodos.c 
> tofrodos-1.7.8.debian.1.new/src/tofrodos.c
> --- tofrodos-1.7.8.debian.1/src/tofrodos.c2011-04-12 18:21:50.0 
> +0200
> +++ tofrodos-1.7.8.debian.1/src/tofrodos.c.new2011-04-12 
> 21:24:27.0 +0200
> @@ -14,7 +14,9 @@
>  #include   /* EXIT_SUCCESS, mkstemp() in some systems */
>  #include   /* strrchr(), strlen(), strcpy(), strcat() */
>  #include /* stat() */
> -
> +#ifdef __GNU__
> +#include 
> +#endif
>  #if defined(UNIX)
>  #if defined(MAXPATHLEN_HEADER)
>  #include MAXPATHLEN_HEADER

Why are you including limits.h, it does not seem to be needed.

> @@ -443,8 +445,12 @@
>  static int openandconvert_preamble ( char * filename )
>  {
>   struct stat statbuf ;
> - charrealfilepath[MAXPATHLEN+1] ;
>   int len ;
> +#ifndef __GNU__
> + charrealfilepath[MAXPATHLEN+1] ;
> +#else
> + char*realfilepath ;
> +#endif

The dynamic memory code should work everywhere, I don't see the need
to complicate things by having more system dependant code paths.

>   /* get the file information */
>   if (lstat( filename, &statbuf )) {
> @@ -461,16 +467,30 @@
>   /* eg, #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK) */
>   /* or something like that. */
>  
> +#ifndef __GNU__
>   if ((len = readlink( filename, realfilepath, 
> (sizeof(realfilepath) - 1) )) != -1) {
>   /* got to null terminate the string - there is always 
> space because */
>   /* we passed readlink() the size of the buffer less 1. 
> */
>   realfilepath[len] = '\0' ;
> +#else
> + realfilepath = realpath (filename, NULL);
> + if ((realfilepath) != NULL) {
> +#endif

So, while doing realpath() would seem to be the correct thing to do
everywhere, as the initial symlink might point to another symlink,
this changes behaviour only for Hurd, which I think it's a bad idea.
But using realpath(foo, NULL) is not really portable and would need to
be checked at build time if it was to be used everywhere.

So what about the attached patch instead (which I had around from some
time ago when I took a look at this, although forgot it in my kvm :/,
beware only build tested).

It also changes the kFreeBSD patch because usually __GLIBC__ is not
predefined and it seems confusing to have it in there. (Ideally the
build system would switch to detect features at build time, instead of
hard-coding system support.)

thanks,
guillem
diff --git a/debian/patches/FTBFS_kfreebsd.diff b/debian/patches/FTBFS_kfreebsd.diff
deleted file mode 100644
index 0cd31b6..000
--- a/debian/patches/FTBFS_kfreebsd.diff
+++ /dev/null
@@ -1,16 +0,0 @@
-## 02_FTBFS_kfreebsd.dpatch by Florian Ernst 
-## All lines beginning with `## DP:' are a description of the patch.
-## DP: Fix FTBFS on GNU/kFreeBSD, patch by Robert Millan, see bug#351417
-
-diff -urNad tofrodos-1.7.6~/src/config.h tofrodos-1.7.6/src/config.h
 tofrodos-1.7.6~/src/config.h	2005-03-15 16:01:02.0 +0100
-+++ tofrodos-1.7.6/src/config.h	2006-02-07 12:01:04.0 +0100
-@@ -95,7 +95,7 @@
- #endif
- 
- /* define the systems */
--#if defined(__linux__)	/* (predefined) */
-+#if defined(__linux__) || defined(__GLIBC__)	/* (predefined) */
- #if !defined(LINUX)
- #define	LINUX
- #endif
diff --git a/debian/patches/FTBFS_non-linux.diff b/debian/patches/FTBFS_non-linux.diff
new file mode 100644
index 000..ef3db9a
--- /dev/null
+++ b/debian/patches/FTBFS_non-linux.diff
@@ -0,0 +1,59 @@
+## 02_FTBFS_non-linux.dpatch by Guillem Jover 
+## All lines beginning with `## DP:' are a description of the patch.
+## DP: Fix FTBFS on non Linux systems
+
+---
+ src/config.h   |4 +++-
+ src/tofrodos.c |   13 ++---
+ 2 files changed, 13 insertions(+), 4 deletions(-)
+
+--- a/src/config.h
 b/src/config.h
+@@ -102,7 +102,9 @@ extern "C" {
+ #endif
+ #endif
+ 
+-#if defined(__FreeBSD__) || defined(__OpenBSD__)	/* seems to work like Linux... */
++#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \
++defined(__OpenBSD__) || defined(__NetBSD__) || \
++defined(__GNU__) /* seems to work like Linux... */
+ #if !defined(LINUX)
+ #define	LINUX
+ #endif
+--- a/src/tofrodos.c
 b/src/tofrodos.c
+@@ -443,7 +443,7 @@ static int convert ( FILE * infp, FILE *
+ static int openandconvert_preamble ( char * filename )
+ {
+ 	struct stat statbuf ;
+-	char		realfilepath[MAXPATHLEN+1] ;
++	char		*realfilepath ;
+ 	int			len ;
+ 
+ 	/* get the file information */
+@@ -461,16 +461,23 @@ static int openandconvert_preamble ( cha
+ 		/* eg, #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK) */
+ 		/* or something like that. */
+ 
+-		if ((len = readlink( filename, realfilepath, (sizeof(realfilepath) - 1) )) != -1) {
++		realfilepath = xmalloc( statbuf.st_size + 1 );
++
++		if

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Samuel Thibault 
Svante Signell, le Tue 12 Apr 2011 21:46:09 +0200, a écrit :
> On Tue, 2011-04-12 at 20:56 +0200, Samuel Thibault wrote:
> > Svante Signell, le Tue 12 Apr 2011 19:27:10 +0200, a écrit :
> > >   return -1 ;
> > >   }
> > >   /* If we reach here, "filename" is not a symbolic link */
> > 
> > And thus we didn't call realpath at all.  I believe the free below
> > shouldn't be done, it's probably only by chance that it doesn't segfault
> > in your test (by being NULL by chance).
> 
> Yes, we did not call realpath but the variable is still declared but not
> malloced. Does that mean we shouldn't free() it?

Of course.

> > > +#ifdef __GNU__
> > > + free (realfilepath);
> > > +#endif
> > >   return openandconvert( filename );
> > >  }
> 
> OK, thanks for your comments. The biggest problem with malloced
> storage, is where to put the free() statements. To do that you need to
> follow the program logic and find out every statement with a possible
> exit. This can be very difficult sometimes, at least I think so.

That's programming...

> Final version??

Seems good to me.

Samuel



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Svante Signell 
On Tue, 2011-04-12 at 20:56 +0200, Samuel Thibault wrote:
> Svante Signell, le Tue 12 Apr 2011 19:27:10 +0200, a écrit :
> > return -1 ;
> > }
> > /* If we reach here, "filename" is not a symbolic link */
> 
> And thus we didn't call realpath at all.  I believe the free below
> shouldn't be done, it's probably only by chance that it doesn't segfault
> in your test (by being NULL by chance).

Yes, we did not call realpath but the variable is still declared but not
malloced. Does that mean we shouldn't free() it?

> > +#ifdef __GNU__
> > +   free (realfilepath);
> > +#endif
> > return openandconvert( filename );
> >  }

OK, thanks for your comments. The biggest problem with malloced
storage, is where to put the free() statements. To do that you need to
follow the program logic and find out every statement with a possible
exit. This can be very difficult sometimes, at least I think so.

Final version??

diff -ur ./tofrodos-1.7.8.debian.1/src/config.h tofrodos-1.7.8.debian.1.new/src/config.h
--- tofrodos-1.7.8.debian.1/src/config.h	2011-04-12 19:02:08.0 +0200
+++ tofrodos-1.7.8.debian.1.new/src/config.h	2011-04-12 19:09:44.0 +0200
@@ -102,7 +102,7 @@
 #endif
 #endif
 
-#if defined(__FreeBSD__) || defined(__OpenBSD__)	/* seems to work like Linux... */
+#if defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__GNU__)	/* seems to work like Linux... */
 #if !defined(LINUX)
 #define	LINUX
 #endif
diff -ur tofrodos-1.7.8.debian.1/src/tofrodos.c tofrodos-1.7.8.debian.1.new/src/tofrodos.c
--- tofrodos-1.7.8.debian.1/src/tofrodos.c	2011-04-12 18:21:50.0 +0200
+++ tofrodos-1.7.8.debian.1/src/tofrodos.c.new	2011-04-12 21:24:27.0 +0200
@@ -14,7 +14,9 @@
 #include 	/* EXIT_SUCCESS, mkstemp() in some systems */
 #include 	/* strrchr(), strlen(), strcpy(), strcat() */
 #include 	/* stat() */
-
+#ifdef __GNU__
+#include 
+#endif
 #if defined(UNIX)
 #if defined(MAXPATHLEN_HEADER)
 #include MAXPATHLEN_HEADER
@@ -443,8 +445,12 @@
 static int openandconvert_preamble ( char * filename )
 {
 	struct stat statbuf ;
-	char		realfilepath[MAXPATHLEN+1] ;
 	int			len ;
+#ifndef __GNU__
+	char		realfilepath[MAXPATHLEN+1] ;
+#else
+	char		*realfilepath ;
+#endif
 
 	/* get the file information */
 	if (lstat( filename, &statbuf )) {
@@ -461,16 +467,30 @@
 		/* eg, #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK) */
 		/* or something like that. */
 
+#ifndef __GNU__
 		if ((len = readlink( filename, realfilepath, (sizeof(realfilepath) - 1) )) != -1) {
 			/* got to null terminate the string - there is always space because */
 			/* we passed readlink() the size of the buffer less 1. */
 			realfilepath[len] = '\0' ;
+#else
+		realfilepath = realpath (filename, NULL);
+		if ((realfilepath) != NULL) {
+#endif
 			if (verbose) {
 emsg( VERBOSE_SYMLINKSRC, filename, realfilepath );
 			}
+#ifndef __GNU__
 			return openandconvert( realfilepath );
+#else
+			len = openandconvert( realfilepath );
+			free (realfilepath);
+			return len;
+#endif
 		}
 		emsg( EMSG_SYMLINK, filename );
+#ifdef __GNU__
+		free (realfilepath);
+#endif
 		return -1 ;
 	}
 	/* If we reach here, "filename" is not a symbolic link */

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Samuel Thibault 
Svante Signell, le Tue 12 Apr 2011 19:27:10 +0200, a écrit :
>   return -1 ;
>   }
>   /* If we reach here, "filename" is not a symbolic link */

And thus we didn't call realpath at all.  I believe the free below
shouldn't be done, it's probably only by chance that it doesn't segfault
in your test (by being NULL by chance).

> +#ifdef __GNU__
> + free (realfilepath);
> +#endif
>   return openandconvert( filename );
>  }

Samuel



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Svante Signell 
On Tue, 2011-04-12 at 17:59 +0200, Svante Signell wrote:
> On Tue, 2011-04-12 at 17:39 +0200, Samuel Thibault wrote:
> > Svante Signell, le Tue 12 Apr 2011 16:33:14 +0200, a écrit :
> > >   if (verbose) {
> > >   emsg( VERBOSE_SYMLINKSRC, filename, 
> > > realfilepath );
> > >   }
> > >   return openandconvert( realfilepath );
> > >   }
> > 
> > You needs to free realfilepath in that case too, so split the return
> > openandconvert() call into an openandconvert call, then free(), then
> > return.
> 
> Ok, new patch will follow shortly.

Attached! Hope the code is better now. It builds and runs OK for me.

diff -ur ./tofrodos-1.7.8.debian.1/src/config.h tofrodos-1.7.8.debian.1.new/src/config.h
--- tofrodos-1.7.8.debian.1/src/config.h	2011-04-12 19:02:08.0 +0200
+++ tofrodos-1.7.8.debian.1.new/src/config.h	2011-04-12 19:09:44.0 +0200
@@ -102,7 +102,7 @@
 #endif
 #endif
 
-#if defined(__FreeBSD__) || defined(__OpenBSD__)	/* seems to work like Linux... */
+#if defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__GNU__)	/* seems to work like Linux... */
 #if !defined(LINUX)
 #define	LINUX
 #endif
diff -ur tofrodos-1.7.8.debian.1/src/tofrodos.c tofrodos-1.7.8.debian.1.new/src/tofrodos.c
--- tofrodos-1.7.8.debian.1/src/tofrodos.c	2008-04-08 06:57:05.0 +0200
+++ tofrodos-1.7.8.debian.1.new/src/tofrodos.c	2011-04-12 18:33:46.0 +0200
@@ -14,7 +14,9 @@
 #include 	/* EXIT_SUCCESS, mkstemp() in some systems */
 #include 	/* strrchr(), strlen(), strcpy(), strcat() */
 #include 	/* stat() */
-
+#ifdef __GNU__
+#include 
+#endif
 #if defined(UNIX)
 #if defined(MAXPATHLEN_HEADER)
 #include MAXPATHLEN_HEADER
@@ -443,8 +445,12 @@
 static int openandconvert_preamble ( char * filename )
 {
 	struct stat statbuf ;
-	char		realfilepath[MAXPATHLEN+1] ;
 	int			len ;
+#ifndef __GNU__
+	char		realfilepath[MAXPATHLEN+1] ;
+#else
+	char		*realfilepath ;
+#endif
 
 	/* get the file information */
 	if (lstat( filename, &statbuf )) {
@@ -461,19 +467,36 @@
 		/* eg, #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK) */
 		/* or something like that. */
 
+#ifndef __GNU__
 		if ((len = readlink( filename, realfilepath, (sizeof(realfilepath) - 1) )) != -1) {
 			/* got to null terminate the string - there is always space because */
 			/* we passed readlink() the size of the buffer less 1. */
 			realfilepath[len] = '\0' ;
+#else
+			realfilepath = realpath (filename, NULL);
+			if ((realfilepath) != NULL) {
+#endif
 			if (verbose) {
 emsg( VERBOSE_SYMLINKSRC, filename, realfilepath );
 			}
+#ifndef __GNU__
 			return openandconvert( realfilepath );
+#else
+			len = openandconvert( realfilepath );
+			free (realfilepath);
+			return len;
+#endif
 		}
 		emsg( EMSG_SYMLINK, filename );
+#ifdef __GNU__
+		free (realfilepath);
+#endif
 		return -1 ;
 	}
 	/* If we reach here, "filename" is not a symbolic link */
+#ifdef __GNU__
+	free (realfilepath);
+#endif
 	return openandconvert( filename );
 }
 #endif

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Svante Signell 
On Tue, 2011-04-12 at 17:39 +0200, Samuel Thibault wrote:
> Svante Signell, le Tue 12 Apr 2011 16:33:14 +0200, a écrit :
> > if (verbose) {
> > emsg( VERBOSE_SYMLINKSRC, filename, 
> > realfilepath );
> > }
> > return openandconvert( realfilepath );
> > }
> 
> You needs to free realfilepath in that case too, so split the return
> openandconvert() call into an openandconvert call, then free(), then
> return.

Ok, new patch will follow shortly.

> When you are unsure about a patch, please post it on debian-hurd@ first
> before submitting it to the BTS.

Will do that next time.





--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Samuel Thibault 
Svante Signell, le Tue 12 Apr 2011 16:33:14 +0200, a écrit :
>   if (verbose) {
>   emsg( VERBOSE_SYMLINKSRC, filename, 
> realfilepath );
>   }
>   return openandconvert( realfilepath );
>   }

You needs to free realfilepath in that case too, so split the return
openandconvert() call into an openandconvert call, then free(), then
return.

When you are unsure about a patch, please post it on debian-hurd@ first
before submitting it to the BTS.

Samuel



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Bug#622365: tofrodos Re: Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Steffen Möller 
You are producing a memory leak. Try not to.
S



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Bug#622365: Fix FTBFS of tofrodos for GNU/Hurd

2011-04-12 Thread Svante Signell 
Package: tofrodos
Version: 1.7.8.debian.1-2
Severity: important
Tags: patch
Usertags: hurd

Attached is a patch to enable successful build of tofrodos on
GNU/Hurd. It should be applied after the FTBFS_kfreebsd.diff patch.
The only question is if I've missed a free() somewhere. In my opinion it
does not make sense to add a free() after a return statement, does it?

Making this package build enables latex2rtf to be built that is on the
old-package list:
http://ftp-master.debian.org/users/twerner/pre-lenny.txt

Additionally, it makes debian-cd installable since it depends on this
package. 
diff -ur ./tofrodos-1.7.8.debian.1/src/config.h tofrodos-1.7.8.debian.1.new/src/config.h
--- ./tofrodos-1.7.8.debian.1/src/config.h	2011-04-12 16:10:02.0 +0200
+++ tofrodos-1.7.8.debian.1.new/src/config.h	2011-04-12 16:18:06.0 +0200
@@ -102,7 +102,7 @@
 #endif
 #endif
 
-#if defined(__FreeBSD__) || defined(__OpenBSD__)	/* seems to work like Linux... */
+#if defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__GNU__)	/* seems to work like Linux... */
 #if !defined(LINUX)
 #define	LINUX
 #endif
diff -ur ./tofrodos-1.7.8.debian.1/src/tofrodos.c tofrodos-1.7.8.debian.1.new/src/tofrodos.c
--- ./tofrodos-1.7.8.debian.1/src/tofrodos.c	2008-04-08 06:57:05.0 +0200
+++ tofrodos-1.7.8.debian.1.new/src/tofrodos.c	2011-04-12 16:22:22.0 +0200
@@ -14,7 +14,9 @@
 #include 	/* EXIT_SUCCESS, mkstemp() in some systems */
 #include 	/* strrchr(), strlen(), strcpy(), strcat() */
 #include 	/* stat() */
-
+#ifdef __GNU__
+#include 
+#endif
 #if defined(UNIX)
 #if defined(MAXPATHLEN_HEADER)
 #include MAXPATHLEN_HEADER
@@ -443,8 +445,12 @@
 static int openandconvert_preamble ( char * filename )
 {
 	struct stat statbuf ;
+#ifndef __GNU__
 	char		realfilepath[MAXPATHLEN+1] ;
 	int			len ;
+#else
+	char		*realfilepath ;
+#endif
 
 	/* get the file information */
 	if (lstat( filename, &statbuf )) {
@@ -461,16 +467,24 @@
 		/* eg, #define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK) */
 		/* or something like that. */
 
+#ifndef __GNU__
 		if ((len = readlink( filename, realfilepath, (sizeof(realfilepath) - 1) )) != -1) {
 			/* got to null terminate the string - there is always space because */
 			/* we passed readlink() the size of the buffer less 1. */
 			realfilepath[len] = '\0' ;
+#else
+			realfilepath = realpath (filename, NULL);
+			if ((realfilepath) != NULL) {
+#endif
 			if (verbose) {
 emsg( VERBOSE_SYMLINKSRC, filename, realfilepath );
 			}
 			return openandconvert( realfilepath );
 		}
 		emsg( EMSG_SYMLINK, filename );
+#ifdef __GNU__
+		free (realfilepath);
+#endif
 		return -1 ;
 	}
 	/* If we reach here, "filename" is not a symbolic link */