Util-Linux Archive on lore.kernel.org
 help / color / Atom feed
From: Karel Zak <kzak@redhat.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: util-linux@vger.kernel.org, Florian Weimer <fweimer@redhat.com>
Subject: Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system
Date: Mon, 19 Aug 2019 15:36:19 +0200
Message-ID: <20190819133619.dtn5ch2sdbme5zir@ws.net.home> (raw)
In-Reply-To: <cover.1565800625.git.ps@pks.im>

On Wed, Aug 14, 2019 at 06:45:03PM +0200, Patrick Steinhardt wrote:
> since commit 52aa1a661 (include/closestream: avoid close more
> than once, 2019-06-13), util-linux fails to build on musl libc
> based systems. The culprit here is that it introduced assignments
> to stderr and stdout, while the C89 standard explicitly notes
> that treating stderr and stdout as valid lvalues is not a
> requirement for any conforming C implementation. musl libc
> implemented these streams as `extern FILE *const`, and as a
> result assigning to these variables causes compiler errors.

The question is if close() for stdout and stderr is the right way to
go. 

In an ideal world it would be enough to use ferror()+fflush(),
unfortunately for example NFS has never reached an ideal world and it
requires fclose()... See

 https://lists.gnu.org/r/bug-gnulib/2019-04/msg00061.html

Florian (added to CC), also suggested to use dup3() for the
descriptors and then fclose() for the new handle. It sounds like a
pretty elegant solution how to avoid all the issues with NULL and it's
also robust enough if you accidentally call close_stream() more than
once.

See

 https://bugzilla.redhat.com/show_bug.cgi?id=1732450#c4

Maybe we can improve include/closestream.h to use dup3(), than it would
be possible keep all in the header file as inline functions. 

Comments?

> Attached is a fix for this. Instead of assigning `NULL` to the
> streams, util-linux now uses a static variable `streams_closed`.

I don't think we need to check if we already performed this operation
as it's always called only once by atexit() and with dup3() it will be
robust enough.

> Unfortunately, this fix necessitated some shifting around as
> closestream was previously implemented as header, only, and
> implementing static variables inside of a header is not going to
> work due to them being static to the single compilation unit,
> only. Thus I converted the code to move the implementation into
> "lib/closestream.c".

 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

  parent reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 16:45 Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 1/4] term-utils/ttymsg: fix missing header for ARRAY_SIZE macro Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 2/4] login-utils/islocal: fix missing header for err macro Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 3/4] lib/closestream: move implementation into its own compilation unit Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 4/4] lib/closestream: fix assignment to read-only standard streams Patrick Steinhardt
2019-08-19 13:36 ` Karel Zak [this message]
2019-08-20 13:17   ` [PATCH 0/4] Fix closing of standard text streams for non-glibc system Patrick Steinhardt
2019-08-20 15:04     ` Karel Zak
2019-08-20 15:11       ` Florian Weimer
2019-08-23 11:52       ` Karel Zak
2019-08-22  9:40 ` [PATCH v2] include/closestream: fix assignment to read-only standard streams Patrick Steinhardt
2019-08-23 12:00   ` Karel Zak
2019-09-02 10:01   ` Karel Zak

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

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

  git send-email \
    --in-reply-to=20190819133619.dtn5ch2sdbme5zir@ws.net.home \
    --to=kzak@redhat.com \
    --cc=fweimer@redhat.com \
    --cc=ps@pks.im \
    --cc=util-linux@vger.kernel.org \
    /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

Util-Linux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/util-linux/0 util-linux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 util-linux util-linux/ https://lore.kernel.org/util-linux \
		util-linux@vger.kernel.org
	public-inbox-index util-linux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.util-linux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git