git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Dan McGregor <dkm560@usask.ca>
To: Duy Nguyen <pclouds@gmail.com>, "McGregor, Dan" <dan.mcgregor@usask.ca>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2] git-compat-util: undefine fileno if defined
Date: Sat, 16 Feb 2019 02:33:27 +0000	[thread overview]
Message-ID: <D8E7C7D0-04E5-4802-80FA-2477F2C0D240@usask.ca> (raw)
In-Reply-To: <20190212134537.GA26137@ash>



On February 12, 2019 7:45:37 a.m. CST, Duy Nguyen <pclouds@gmail.com> wrote:
>On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote:
>> Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR",
>> 2019-01-10) introduced an implicit assumption that rewind, fileno,
>and
>> fflush are functions. At least on FreeBSD fileno is not, and as such
>> passing a void * failed.
>> 
>> All systems tested (FreeBSD and NetBSD) that define fineo as a macro
>
>OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while
>NetBSD compiles ok (and fails to run some tests)
>
>[1] https://gitlab.com/git-vcs/git-ci/pipelines/47139741/failures
>
>> also have a function defined. Undefine the macro on these systems so
>> that the function is used.
>
>I don't think this is enough. At least fbsd defines this
>
>#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))
>
>so one of the two functions will be used depending on __isthreaded
>flag. Your forcing to use fileno, ignoring __sfileno, is technically
>not correct.
>
>For the record, at least fbsd also defines feof, ferror, clearerr,
>getc and putc in the same way. But at least I don't see how something
>like feof(fp++) could cause bad side effects.
>
>So, how about something like this? A teeny bit longer than your
>version, but I think it's easier to control long term.

Yes, this looks pretty reasonable to me too.
>
>-- 8< --
>diff --git a/Makefile b/Makefile
>index 0e13a5b469..44cfc63fc4 100644
>--- a/Makefile
>+++ b/Makefile
>@@ -433,6 +433,8 @@ all::
> #
> # Define HAVE_GETDELIM if your system has the getdelim() function.
> #
>+# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real
>function.
>+#
> # Define PAGER_ENV to a SP separated VAR=VAL pairs to define
># default environment variables to be passed when a pager is spawned,
>e.g.
> #
>@@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR
> 	BASIC_CFLAGS += -DHAVE_WPGMPTR
> endif
> 
>+ifdef FILENO_IS_A_MACRO
>+	COMPAT_CFLAGS += -DFILENO_IS_A_MACRO
>+	COMPAT_OBJS += compat/fileno.o
>+endif
>+
> ifeq ($(TCLTK_PATH),)
> NO_TCLTK = NoThanks
> endif
>diff --git a/compat/fileno.c b/compat/fileno.c
>new file mode 100644
>index 0000000000..7b105f4cd7
>--- /dev/null
>+++ b/compat/fileno.c
>@@ -0,0 +1,7 @@
>+#define COMPAT_CODE
>+#include "../git-compat-util.h"
>+
>+int git_fileno(FILE *stream)
>+{
>+	return fileno(stream);
>+}
>diff --git a/config.mak.uname b/config.mak.uname
>index 786bb2f913..01f62674a4 100644
>--- a/config.mak.uname
>+++ b/config.mak.uname
>@@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD)
> 	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
> 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
> 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>+	FILENO_IS_A_MACRO = UnfortunatelyYes
> endif
> ifeq ($(uname_S),OpenBSD)
> 	NO_STRCASESTR = YesPlease
>@@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD)
> 	HAVE_BSD_KERN_PROC_SYSCTL = YesPlease
> 	PROCFS_EXECUTABLE_PATH = /proc/curproc/file
> 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>+	FILENO_IS_A_MACRO = UnfortunatelyYes
> endif
> ifeq ($(uname_S),MirBSD)
> 	NO_STRCASESTR = YesPlease
>diff --git a/git-compat-util.h b/git-compat-util.h
>index 29a19902aa..6573808ebd 100644
>--- a/git-compat-util.h
>+++ b/git-compat-util.h
>@@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct
>tm *);
> #define getc_unlocked(fh) getc(fh)
> #endif
> 
>+#ifdef FILENO_IS_A_MACRO
>+int git_fileno(FILE *stream);
>+# ifndef COMPAT_CODE
>+#  undef fileno
>+#  define fileno(p) git_fileno(p)
>+# endif
>+#endif
>+
> /*
>  * Our code often opens a path to an optional file, to work on its
>  * contents when we can successfully open it.  We can ignore a failure
>-- 8< --
>
>--
>Duy

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  parent reply	other threads:[~2019-02-16  2:33 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-01 19:30 [PATCH] http: cast result to FILE * Dan McGregor
2019-02-01 21:17 ` Junio C Hamano
2019-02-02 11:21   ` Duy Nguyen
2019-02-04 11:44     ` Johannes Schindelin
2019-02-04 12:13       ` Duy Nguyen
2019-02-04 18:10         ` Junio C Hamano
2019-02-05  1:36           ` Dan McGregor
2019-02-09  2:36 ` [PATCH v2] git-compat-util: undefine fileno if defined Dan McGregor
2019-02-09 18:34   ` Junio C Hamano
2019-02-12 13:45   ` Duy Nguyen
2019-02-12 14:14     ` Duy Nguyen
2019-02-12 16:56     ` Junio C Hamano
2019-02-16  2:33     ` Dan McGregor [this message]
2019-05-08  7:46       ` Junio C Hamano
2019-05-08 10:09         ` Duy Nguyen
2019-05-08 10:16           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D8E7C7D0-04E5-4802-80FA-2477F2C0D240@usask.ca \
    --to=dkm560@usask.ca \
    --cc=dan.mcgregor@usask.ca \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).