Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

[Phillip Wood <phillip.wood123@gmail.com>]

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.

> 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).

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.

> 
>>> In this case I think it would make sense to skip "error: " from the
>>> message itself.
>>> Eventually we'll get to making usage.c have that prefix translated,
>>> and
>>> can have some utility function exposed there (I have WIP patches for
>>> this already since a while ago).
>>> To translators it'll look like the same thing, and avoid churn when
>>> we
>>> make the "error: " prefix translatable.
>>
>> Unless we add a function that returns a string rather than printing
>> the message I don't see how it avoids churn in the future. Having the
>> whole string with the "error: " prefix translated here does not add
>> any extra burden to translators - it is still the same number of
>> strings to translate.
> 
> Because translators translate "we failed" for most errors, not "error:
> we failed".
> 
> If and when we convert it from "error: we failed" to "we failed" they'll
> need to translate it again (although to be fair, the translation cache
> will help).
> 
> And even then it'll be one of very few exceptions where the "error: "
> currently that *is* translated.
> 
>>> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above
>>> advice...) doesn't it make sense to add the "\n" here, so you'll have
>>> one write_in_full() above?
>>
>> I decided to keep the translated string simpler by omitting the
>> newline, calling write_in_full() twice isn't a bit deal (I don't think
>> the output can be split by a write from another thread or signal
>> handler in between).
> 
> Makes sense.
> 
> FWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you
> could do:
> 
>      xstrfmt("error: %s\n", _("the error"))
> 
> And then do one call to write_in_full().
> 
> But I think just:
> 
>      msg = _("the error");
> 
> And then:
> 
> 	const char *const = pfx = "error: ";
>          const size_t len = strlen(pfx);
> 
> 	write_in_full(2, pfx, len);
>          write_in_full(2, msg, strlen(msg));
> 	write_in_full(2, "\n", 1);
> 
> Makes more sense :)

Agreed, I'll change that.

Best Wishes

Phillip