On 07/03/2022 14:45, Ævar Arnfjörð Bjarmason wrote:
[...]
On Mon, Mar 07 2022, Phillip Wood wrote:
On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:
On Mon, Mar 07 2022, Phillip Wood wrote:
Hi Ævar
On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:
[...]
int save_term(unsigned flags)
{
+ struct sigaction sa;
+
if (term_fd < 0)
term_fd = (flags & SAVE_TERM_STDIN) ? 0
: open("/dev/tty", O_RDWR);
@@ -44,6 +136,26 @@ int save_term(unsigned flags)
if (tcgetattr(term_fd, &old_term) < 0)
return -1;
sigchain_push_common(restore_term_on_signal);
+ /*
+ * If job control is disabled then the shell will have set the
+ * disposition of SIGTSTP to SIG_IGN.
+ */
+ sigaction(SIGTSTP, NULL, &sa);
+ if (sa.sa_handler == SIG_IGN)
+ return 0;
+
+ /* avoid calling gettext() from signal handler */
+ background_resume_msg = xstrdup(_("error: cannot resume in the background"));
+ restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
You don't need to xstrdup() the return values of gettext() (here
_()),
you'll get a pointer to static storage that's safe to hold on to for the
duration of the program.
I had a look at the documentation and could not see anything about the
lifetime of the returned string, all it says is "don't alter it"
I think this is overed in "11.2.7 Optimization of the *gettext
functions", a pedantic reading might suggest not, but what's meant with
the combination of that API documentation & the description of how MO
files work is that you're just getting pointers into the already-loaded
translation catalog, so it's safe to hold onto the pointer and re-use it
later.
Strictly that section only shows it is safe if there are no other
calls to gettext() before the returned string is used. I agree the
implementation is likely to be just returning static strings but I
can't find anywhere that says another implementation (e.g. on
macos/*bsd) has to do that.
I agree. I'm 99.99% sure this is safe & portable use of the API, but I'm
having some trouble finding documentation for that...
In any case, if we're going to be paranoid about gettext() it would make
sense to propose that as some general change to how we use it, we rely
on this assumption holding in a lot of our use of the API:
git grep '= _\('
Rather than sneak that partcular new assumption in here in this
already
tricky code...
The ones I looked at are mostly not calling gettext() again before
using the translated string (there is one exception in
builtin/remote.c).
Doesn't validate_encoding() in convert.c, process_entry() in
merge-ort.c, setup_unpack_trees_porcelain() in unpack-trees.c cmd_mv()
in builtin/mv.c etc. qualify?
I only checked a few, cmd_mv() always assigns to the same variable so
the previous value is overwritten anyway, some of the others such as
unpack_trees are assuming the return value is valid after a subsequent
call to gettext(). I found[1] which states
The string returned must not be modified by the program and can
be invalidated by a subsequent call to bind_textdomain_codeset()
or setlocale(3C).
so I think we can drop the copying.
I.e. for a hypothetical gettext() that always returned the same pointer
and just overwrote it with the latest message those would all emit bad
output, wouldn't they?
In restore_term() I'm checking if the messages are NULL to see if job
control is enabled, I could use a flag but I'm inclined to just keep
coping the strings.
Checking if they're NULL is orthagonal to whether we xstrdup()
them. I.e. you'd just skip the xstrdup() and replace the FREE_AND_NULL
with a "= NULL" assignment, no?
Yes, I'm not sure what I was thinking when I wrote that.
Best Wishes
Phillip
[1]
https://docs.oracle.com/cd/E88353_01/html/E37843/gettext-3c.html#REFMAN3Agettext-3c
