Re: bug: "fatal: cannot pread pack file", version 1.7.5.4

Jeff King [off-list ref] writes:

It's very odd for pread to report ENOENT (since it is always operating
on an already-opened file descriptor).

It doesn't, but gettext will clobber errno:

		n = pread(pack_fd, inbuf, n, from);
		if (n < 0)
			die_errno(_("cannot pread pack file"));

There is nothing that saves errno.  This isn't limited to i18n though,
any function call in the arguments may potentially clobber errno.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

On Tue, Jul 03, 2012 at 12:43:42AM +0200, Andreas Schwab wrote:

Jeff King [off-list ref] writes:

quoted

It's very odd for pread to report ENOENT (since it is always operating
on an already-opened file descriptor).

It doesn't, but gettext will clobber errno:

		n = pread(pack_fd, inbuf, n, from);
		if (n < 0)
			die_errno(_("cannot pread pack file"));

There is nothing that saves errno.  This isn't limited to i18n though,
any function call in the arguments may potentially clobber errno.

That's horribly lame of gettext. I don't expect arbitrary functions to
save errno, but when the entire purpose of a function is to be a
non-intrusive wrapper to massage messages to the user, it seems kind of
evil to overwrite errno. Isn't the whole point of calling it "_" that
you don't want to or have to notice it?

Can we do something like this to get around it?

diff --git a/gettext.h b/gettext.h
index 57ba8bb..b7c3ae5 100644
--- a/gettext.h
+++ b/gettext.h

@@ -44,7 +44,10 @@ extern int use_gettext_poison(void);
 
 static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
 {
-	return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
+	int saved_errno = errno;
+	const char *r = use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
+	errno = saved_errno;
+	return r;
 }
 
 static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)

On Tue, Jul 3, 2012 at 10:45 AM, Jeff King [off-list ref] wrote:

On Tue, Jul 03, 2012 at 12:43:42AM +0200, Andreas Schwab wrote:

quoted

Jeff King [off-list ref] writes:

quoted

It's very odd for pread to report ENOENT (since it is always operating
on an already-opened file descriptor).

It doesn't, but gettext will clobber errno:

              n = pread(pack_fd, inbuf, n, from);
              if (n < 0)
                      die_errno(_("cannot pread pack file"));

There is nothing that saves errno.  This isn't limited to i18n though,
any function call in the arguments may potentially clobber errno.

That's horribly lame of gettext. I don't expect arbitrary functions to
save errno, but when the entire purpose of a function is to be a
non-intrusive wrapper to massage messages to the user, it seems kind of
evil to overwrite errno. Isn't the whole point of calling it "_" that
you don't want to or have to notice it?

Agreed.

The last line belongs to Q_(), which needs the same treatment.
-- 
Duy

On Tue, Jul 03, 2012 at 11:25:16AM +0700, Nguyen Thai Ngoc Duy wrote:

On Tue, Jul 3, 2012 at 10:45 AM, Jeff King [off-list ref] wrote:

quoted

On Tue, Jul 03, 2012 at 12:43:42AM +0200, Andreas Schwab wrote:

quoted

Jeff King [off-list ref] writes:

quoted

It's very odd for pread to report ENOENT (since it is always operating
on an already-opened file descriptor).

It doesn't, but gettext will clobber errno:

              n = pread(pack_fd, inbuf, n, from);
              if (n < 0)
                      die_errno(_("cannot pread pack file"));

There is nothing that saves errno.  This isn't limited to i18n though,
any function call in the arguments may potentially clobber errno.

That's horribly lame of gettext. I don't expect arbitrary functions to
save errno, but when the entire purpose of a function is to be a
non-intrusive wrapper to massage messages to the user, it seems kind of
evil to overwrite errno. Isn't the whole point of calling it "_" that
you don't want to or have to notice it?

Agreed.

Hmm. According to gettext(3):

  ERRORS
         errno is not modified.

And googling for "gettext" and "errno" reveals several bugfixes in GNU
gettext to make sure that gettext preserves errno. I wonder if there are
systems where that is not the case, though; I don't know what non-GNU
gettext implementations are in common use these days. I'd still be
curious to hear what platform the server is for this bug report.

-Peff