The motivation is for the SUPDUP protocol. The server may send a
signal for the terminal to reset any input buffers. After this, the
server will not know the state of the terminal, so it is required to
send its cursor position back.
Sometimes, within a switch statement, you want to declare local
variables specific to the handler for one particular case. Until now
I've mostly been writing this in the form
switch (discriminant) {
case SIMPLE:
do stuff;
break;
case COMPLICATED:
{
declare variables;
do stuff;
}
break;
}
which is ugly because the two pieces of essentially similar code
appear at different indent levels, and also inconvenient because you
have less horizontal space available to write the complicated case
handler in - particuarly undesirable because _complicated_ case
handlers are the ones most likely to need all the space they can get!
After encountering a rather nicer idiom in the LLVM source code, and
after a bit of hackery this morning figuring out how to persuade
Emacs's auto-indent to do what I wanted with it, I've decided to move
to an idiom in which the open brace comes right after the case
statement, and the code within it is indented the same as it would
have been without the brace. Then the whole case handler (including
the break) lives inside those braces, and you get something that looks
more like this:
switch (discriminant) {
case SIMPLE:
do stuff;
break;
case COMPLICATED: {
declare variables;
do stuff;
break;
}
}
This commit is a big-bang change that reformats all the complicated
case handlers I could find into the new layout. This is particularly
nice in the Pageant main function, in which almost _every_ case
handler had a bundle of variables and was long and complicated. (In
fact that's what motivated me to get round to this.) Some of the
innermost parts of the terminal escape-sequence handling are also
breathing a bit easier now the horizontal pressure on them is
relieved.
(Also, in a few cases, I was able to remove the extra braces
completely, because the only variable local to the case handler was a
loop variable which our new C99 policy allows me to move into the
initialiser clause of its for statement.)
Viewed with whitespace ignored, this is not too disruptive a change.
Downstream patches that conflict with it may need to be reapplied
using --ignore-whitespace or similar.
In the previous trawl of this, I didn't bother with the statics in
main-program modules, on the grounds that my main aim was to avoid
'library' objects (shared between multiple programs) from polluting
the global namespace. But I think it's worth being more strict after
all, so this commit adds 'static' to a lot more file-scope variables
that aren't needed outside their own module.
Now it's no longer used, we can get rid of it, and better still, get
rid of every #define PUTTY_DO_GLOBALS in the many source files that
previously had them.
We now have no remaining things in header files that switch from being
a declaration to a definition depending on an awkward #define at the
point of including that header. There are still a few mutable
variables with external linkage, but at least now each one is defined
in a specific source file file appropriate to its purpose and context.
The remaining globals as of this commit were:
- 'logctx' and 'term', which never needed to be globals in the first
place, because they were never actually shared between source
files. Now 'term' is just a static in window.c, and 'logctx' is a
static in each of that and winplink.c.
- 'hinst', which still has external linkage, but is now defined
separately in each source file that sets it up (i.e. those with a
WinMain)
- osMajorVersion, osMinorVersion and osPlatformId, whose definitions
now live in winmisc.c alongside the code which sets them up.
(Actually they were defined there all along, it turns out, but
every toolchain I've built with has commoned them together with the
version defined by the GLOBAL in the header.)
- 'hwnd', which nothing was actually _using_ any more after previous
commits, so all this commit had to do was delete it.
This was the difficult part of cleaning up that global variable. The
main Windows PuTTY GUI is split between source files, so that _does_
actually need to refer to the main window from multiple places.
But all the places where windlg.c needed to use 'hwnd' are seat
methods, so they were already receiving a Seat pointer as a parameter.
In other words, the methods of the Windows GUI Seat were already split
between source files. So it seems only fair that they should be able
to share knowledge of the seat's data as well.
Hence, I've created a small 'WinGuiSeat' structure which both window.c
and windlg.c can see the layout of, and put the main terminal window
handle in there. Then the seat methods implemented in windlg.c, like
win_seat_verify_ssh_host_key, can use container_of to turn the Seat
pointer parameter back into the address of that structure, just as the
methods in window.c can do (even though they currently don't need to).
(Who knows: now that it _exists_, perhaps that structure can be
gradually expanded in future to turn it into a proper encapsulation of
all the Windows frontend's state, like we should have had all
along...)
I've also moved the Windows GUI LogPolicy implementation into the same
object (i.e. WinGuiSeat implements both traits at once). That allows
win_gui_logging_error to recover the same WinGuiSeat from its input
LogPolicy pointer, which means it can get from there to the Seat facet
of the same object, so that I don't need the extern variable
'win_seat' any more either.
The GUI version of pgp_fingerprints() is now a differently named
function that takes a parent HWND as a parameter, and so does my
help-enabled wrapper around MessageBox.
It's now a static in the main source file of each application that
uses it, and isn't accessible from any other source file unless the
main one passes it by reference.
In fact, there were almost no instances of the latter: only the
config-box functions in windlg.c were using 'conf' by virtue of its
globalness, and it's easy to make those take it as a parameter.
It's silly to set it at each call site of restrict_process_acl() if
that function returns success! More sensible to have it be a flag in
the same source file as restrict_process_acl(), set as an automatic
_side effect_ of success.
I've renamed the variable itself, and the global name 'restricted_acl'
is now a query function that asks winsecur.c whether that operation
has been (successfully) performed.
These global variables are only ever used by load_settings, which uses
them to vary the default protocol and port number in the absence of
any specification elsewhere. So there's no real need for them to be
universally accessible via the awkward GLOBAL mechanism: they can be
statics inside settings.c, with accessor functions that can set them.
That was the last GLOBAL in putty.h, so I've removed the definition of
the macro GLOBAL itself as well. There are still some GLOBALs in the
Windows subdirectory, though.
Another ugly mutable global variable gone: now, instead of this
variable being defined in cmdline.c and written to by everyone's
main(), it's defined _alongside_ everyone's main() as a constant, and
cmdline.c just refers to it.
A bonus is that now nocmdline.c doesn't have to define it anyway for
tools that don't use cmdline.c. But mostly, it didn't need to be
mutable, so better for it not to be.
While I'm at it, I've also fiddled with the bit flags that go in it,
to define their values automatically using a list macro instead of
manually specifying each one to be a different power of 2.
This is another piece of the old 2003 attempt at async agent requests.
Nothing ever calls this function (in particular, the new working
version of async-agent doesn't need it). Remove it completely, and all
its special-window-message implementations too.
(If we _were_ still using this function, then it would surely be
possible to fold it into the more recently introduced general
toplevel-callback system, and get rid of all this single-use special
code. But we're not, so removing it completely is even easier.)
In particular, this system was the only reason why Windows Plink paid
any attention to its message queue. So now I can make it call plain
WaitForMultipleObjects instead of MsgWaitForMultipleObjects.
The global 'int flags' has always been an ugly feature of this code
base, and I suddenly thought that perhaps it's time to start throwing
it out, one flag at a time, until it's totally unused.
My first target is FLAG_VERBOSE. This was usually set by cmdline.c
when it saw a -v option on the program's command line, except that GUI
PuTTY itself sets it unconditionally on startup. And then various bits
of the code would check it in order to decide whether to print a given
message.
In the current system of front-end abstraction traits, there's no
_one_ place that I can move it to. But there are two: every place that
checked FLAG_VERBOSE has access to either a Seat or a LogPolicy. So
now each of those traits has a query method for 'do I want verbose
messages?'.
A good effect of this is that subsidiary Seats, like the ones used in
Uppity for the main SSH server module itself and the server end of
shell channels, now get to have their own verbosity setting instead of
inheriting the one global one. In fact I don't expect any code using
those Seats to be generating any messages at all, but if that changes
later, we'll have a way to control it. (Who knows, perhaps logging in
Uppity might become a thing.)
As part of this cleanup, I've added a new flag to cmdline_tooltype,
called TOOLTYPE_NO_VERBOSE_OPTION. The unconditionally-verbose tools
now set that, and it has the effect of making cmdline.c disallow -v
completely. So where 'putty -v' would previously have been silently
ignored ("I was already verbose"), it's now an error, reminding you
that that option doesn't actually do anything.
Finally, the 'default_logpolicy' provided by uxcons.c and wincons.c
(with identical definitions) has had to move into a new file of its
own, because now it has to ask cmdline.c for the verbosity setting as
well as asking console.c for the rest of its methods. So there's a new
file clicons.c which can only be included by programs that link
against both cmdline.c _and_ one of the *cons.c, and I've renamed the
logpolicy to reflect that.
This was pointed out as a compiler warning when I test-built with
up-to-date clang-cl. It looks as if it would cause the IDM_FULLSCREEN
item on the system menu to be wrongly greyed/ungreyed, but in fact I
think it's benign, because MF_BYCOMMAND == 0. So it's _just_ a
warning fix, luckily!
Windows Plink and PSFTP had very similar implementations, and now they
share one that lives in a new file winselcli.c. I've similarly moved
GUI PuTTY's implementation out of window.c into winselgui.c, where
other GUI programs wanting to do networking will be able to access
that too.
In the spirit of centralisation, I've also taken the opportunity to
make both functions use the reasonably complete winsock_error_string()
rather than (for some historical reason) each inlining a minimal
version that reports most errors as 'unknown'.
The do_select function is called with a boolean parameter indicating
whether we're supposed to start or stop paying attention to network
activity on a given socket. So if we freeze and unfreeze the socket in
mid-session because of backlog, we'll call do_select(s, false) to
freeze it, and do_select(s, true) to unfreeze it.
But the implementation of do_select in the Windows SFTP code predated
the rigorous handling of socket backlogs, so it assumed that
do_select(s, true) would only be called at initialisation time, i.e.
only once, and therefore that it was safe to use that flag as a cue to
set up the Windows event object to associate with socket activity.
Hence, every time the socket was frozen and unfrozen, we would create
a new netevent at unfreeze time, leaking the old one.
I think perhaps part of the reason why that was hard to figure out was
that the boolean parameter was called 'startup' rather than 'enable'.
To make it less confusing the next time I read this code, I've also
renamed it, and while I was at it, adjusted another related comment.
The number of people has been steadily increasing who read our source
code with an editor that thinks tab stops are 4 spaces apart, as
opposed to the traditional tty-derived 8 that the PuTTY code expects.
So I've been wondering for ages about just fixing it, and switching to
a spaces-only policy throughout the code. And I recently found out
about 'git blame -w', which should make this change not too disruptive
for the purposes of source-control archaeology; so perhaps now is the
time.
While I'm at it, I've also taken the opportunity to remove all the
trailing spaces from source lines (on the basis that git dislikes
them, and is the only thing that seems to have a strong opinion one
way or the other).
Apologies to anyone downstream of this code who has complicated patch
sets to rebase past this change. I don't intend it to be needed again.
The RESIZE_EITHER resizing mode responds to a window resize by
changing the logical terminal size if the window is shown normally, or
by changing the font size to keep the terminal size the same if the
resize is a transition between normal and maximised state.
But a user pointed out that it's also possible for a window to receive
a WM_SIZE message while _remaining_ in maximised state, and that
PuTTY's resize logic didn't allow for that possibility. It occurs when
there's a change in the amount of available screen space for the
window to be maximised _in_: e.g. when the video resolution is
reconfigured, or when you reconnect to a Remote Desktop session using
a client window of a different size, or even when you toggle the
'Automatically hide the taskbar' option in the Windows taskbar settings.
In that situation, the right thing seems to be for PuTTY to continue
to go with the policy of changing the font size rather than the
logical terminal size. In other words, we prefer to change the font
size when the resize is _from_ maximised state, _to_ maximised state,
_or both_.
That's easily implemented by removing the check of the 'was_zoomed'
flag, in the case where we've received a WM_SIZE message with the
state SIZE_MAXIMIZED: once we know the transition is _to_ maximised
state, it doesn't matter whether or not it was also _from_ it. (But we
still set the was_zoomed flag to the most recent maximised status, so
that we can recognise transitions _out_ of maximised mode.)
The functions that previously lived in it now live in terminal.c
itself; they've been renamed term_keyinput and term_keyinputw, and
their function is to add data to the terminal's user input buffer from
a char or wchar_t string respectively.
They sit more comfortably in terminal.c anyway, because their whole
point is to translate into the character encoding that the terminal is
currently configured to use. Also, making them part of the terminal
code means they can also take care of calling term_seen_key_event(),
which simplifies most of the call sites in the GTK and Windows front
ends.
Generation of text _inside_ terminal.c, from responses to query escape
sequences, is therefore not done by calling those external entry
points: we send those responses directly to the ldisc, so that they
don't count as keypresses for all the user-facing purposes like bell
overload handling and scrollback reset. To make _that_ convenient,
I've arranged that most of the code that previously lived in
lpage_send and luni_send is now in separate translation functions, so
those can still be called from situations where you're not going to do
the default thing with the translated data.
(However, pasted data _does_ still count as close enough to a keypress
to call term_seen_key_event - but it clears the 'interactive' flag
when the data is passed on to the line discipline, which tweaks a
minor detail of control-char handling in line ending mode but mostly
just means pastes aren't interrupted.)
The recent rewriting in both the GTK and Windows keyboard handlers
left the keypad 'Enter' key in a bad state, when no override is
enabled that causes it to generate an escape sequence.
On Windows, a series of fallbacks was causing it to generate \r
regardless of configuration, whereas in Telnet mode it should default
to generating the special Telnet new-line sequence, and in response to
ESC[20h (enabling term->cr_lf_return) it should generate \r\n.
On GTK, it wasn't generating anything _at all_, and also, I can't see
any evidence that the GTK keyboard handler had ever remembered to
implement the cr_lf_return mode.
Now Keypad Enter in non-escape-sequence mode should behave just like
Return, on both platforms.
TranslateKey() on Windows passed all numeric-keypad key events to this
function in terminal.c, and accepted whatever it gave back. That
included the handling for the trivial case of the numeric keypad, when
Num Lock is on and application keypad mode hasn't overridden it, so
that the keypad should be returning actual digits. In that case,
format_numeric_keypad_key() itself was returning the same ASCII
character I had passed in to it as a keypad identifier, and
TranslateKey was returning that in turn as the final translation.
Unfortunately, that means that with Num Lock on, the numeric keypad
translates into what _I_ used as the logical keypad codes inside the
source code, not what the local keyboard layout thinks are the right
codes. In particular, the key I identified as keypad '.' would render
as '.' even on a German keyboard where it ought to produce ','.
Fixed by removing the fallback case in format_numeric_keypad_key()
itself, so now it returns the empty string if it didn't produce an
escape sequence as its translation. Instead, the special case is in
window.c, which checks for a zero-length output string and handles it
by falling through to the keyboard-layout specific ToUnicode code
further down TranslateKey().
On the GTK side, no change is needed here: the GTK keyboard handler
does things in the opposite order, by trying the local input method
_first_ (unless it can see a reason up front to override it), and only
calling format_numeric_keypad_key() if that didn't provide a
translation. So the fallback ASCII translation in the latter was
already not used.
Re-consider the icon in light of the font size, so that we pick the icon
whose size mostly closely matches the terminal font, rather than always
scaling the default icon.
In my eagerness to make sure we didn't _accidentally_ change the
seat's trust status back to trusted at any point, I forgot to do it on
purpose if a second SSH login phase is legitimately run in the same
terminal after the first session has ended.
In terminal-based GUI applications, this is passed through to
term_set_trust_status, to toggle whether lines are prefixed with the
new trust sigil. In console applications, the function returns false,
indicating to the backend that it should employ some other technique
for spoofing protection.
This is not yet used by anything, but the idea is that it'll be a
graphic in the terminal window that can't be replicated by a server
sending escape sequences, and hence can be used as a reliable
indication that the text on a particular terminal line is generated by
PuTTY itself and not passed through from the server. This will make it
possible to detect a malicious server trying to mimic local prompts to
trick you out of information that shouldn't be sent over the wire
(such as private-key passphrases).
The trust sigil I've picked is a small copy of the PuTTY icon, which
is thematically nice (it can be read as if the PuTTY icon is the name
of the speaker in a dialogue) and also convenient because we had that
graphic available already on all platforms. (Though the contortions I
had to go through to make the GTK 1 code draw it were quite annoying.)
The trust sigil has the same dimensions as a CJK double-width
character, i.e. it's 2 character cells wide by 1 high.
Now instead of taking raw arguments to configure the output
StripCtrlChars with, it takes an enumerated value giving the context
of what's being sanitised, and allows the seat to decide what the
output parameters for that context should be.
The only context currently used is SIC_BANNER (SSH login banners).
I've also added a not-yet-used one for keyboard-interactive prompts.
If centralised code like the SSH implementation wants to sanitise
escape sequences out of a piece of server-provided text, it will need
to do it by making a locale-based StripCtrlChars if it's running in a
console context, or a Terminal-based one if it's in a GUI terminal-
window application.
All the other changes of behaviour needed between those two contexts
are handled by providing reconfigurable methods in the Seat vtable;
this one is no different. So now there's a new method in the Seat
vtable that will construct a StripCtrlChars appropriate to that kind
of seat. Terminal-window seats (gtkwin.c, window.c) implement it by
calling the new stripctrl_new_term(), and console ones use the locale-
based stripctrl_new().
The idea of these is that they centralise the common idiom along the
lines of
if (logical_array_len >= physical_array_size) {
physical_array_size = logical_array_len * 5 / 4 + 256;
array = sresize(array, physical_array_size, ElementType);
}
which happens at a zillion call sites throughout this code base, with
different random choices of the geometric factor and additive
constant, sometimes forgetting them completely, and generally doing a
lot of repeated work.
The new macro sgrowarray(array,size,n) has the semantics: here are the
array pointer and its physical size for you to modify, now please
ensure that the nth element exists, so I can write into it. And
sgrowarrayn(array,size,n,m) is the same except that it ensures that
the array has size at least n+m (so sgrowarray is just the special
case where m=1).
Now that this is a single centralised implementation that will be used
everywhere, I've also gone to more effort in the implementation, with
careful overflow checks that would have been painful to put at all the
previous call sites.
This commit also switches over every use of sresize(), apart from a
few where I really didn't think it would gain anything. A consequence
of that is that a lot of array-size variables have to have their types
changed to size_t, because the macros require that (they address-take
the size to pass to the underlying function).
I've fixed a handful of these where I found them in passing, but when
I went systematically looking, there were a lot more that I hadn't
found!
A particular highlight of this collection is the code that formats
Windows clipboard data in RTF, which was absolutely crying out for
strbuf_catf, and now it's got it.
This is a general cleanup which has been overdue for some time: lots
of length fields are now the machine word type rather than the (in
practice) fixed 'int'.
The upcoming PRNG revamp will want to tell noise sources apart, so
that it can treat them all fairly. So I've added an extra parameter to
noise_ultralight and random_add_noise, which takes values in an
enumeration covering all the vague classes of entropy source I'm
collecting. In this commit, though, it's simply ignored.
Taking a leaf out of the LLVM code base: this macro still includes an
assert(false) so that the message will show up in a typical build, but
it follows it up with a call to a function explicitly marked as no-
return.
So this ought to do a better job of convincing compilers that once a
code path hits this function it _really doesn't_ have to still faff
about with making up a bogus return value or filling in a variable
that 'might be used uninitialised' in the following code that won't be
reached anyway.
I've gone through the existing code looking for the assert(false) /
assert(0) idiom and replaced all the ones I found with the new macro,
which also meant I could remove a few pointless return statements and
variable initialisations that I'd already had to put in to placate
compiler front ends.
In the previous commit I happened to notice a %.150s in a ppl_logevent
call, which was probably an important safety precaution a couple of
decades ago when that format string was being used for an sprintf into
a fixed-size buffer, but now it's just pointless cruft.
This commit removes all printf string formatting directives with a
compile-time fixed size, with the one exception of a %.3s used to cut
out a 3-letter month name in scpserver.c. In cases where the format
string in question was already going to an arbitrary-length function
like dupprintf or ppl_logevent, that's all I've done; in cases where
there was still a fixed-size buffer, I've replaced it with a dynamic
buffer and dupprintf.
In the past, I've had a lot of macros which you call with double
parentheses, along the lines of debug(("format string", params)), so
that the inner parens protect the commas and permit the macro to treat
the whole printf-style argument list as one macro argument.
That's all very well, but it's a bit inconvenient (it doesn't leave
you any way to implement such a macro by prepending another argument
to the list), and now this code base's rules allow C99isms, I can
switch all those macros to using a single pair of parens, using the
C99 ability to say '...' in the parameter list of the #define and get
at the corresponding suffix of the arguments as __VA_ARGS__.
So I'm doing it. I've made the following printf-style macros variadic:
bpp_logevent, ppl_logevent, ppl_printf and debug.
While I'm here, I've also fixed up a collection of conditioned-out
calls to debug() in the Windows front end which were clearly expecting
a macro with a different calling syntax, because they had an integer
parameter first. If I ever have a need to condition those back in,
they should actually work now.
A long time ago, in commit 4d77b6567, I moved the generation of the
arrow-key escape sequences into a function format_arrow_key(). Mostly
the reason for that was a special purpose I had in mind at the time
which involved auto-generating the same sequences in response to
things other than a keypress, but I always thought it would be nice to
centralise a lot more of PuTTY's complicated keyboard handling in the
same way - at least the handling of the function keys and their
numerous static and dynamic config options.
In this year's general spirit of tidying up and refactoring, I think
it's finally time. So here I introduce three more centralised
functions for dealing with the numbered function keys, the small
keypad (Ins, Home, PgUp etc) and the numeric keypad. Lots of horrible
and duplicated code from the key handling functions in window.c and
gtkwin.c is now more sensibly centralised: each platform keyboard
handler concerns itself with the local format of a keyboard event and
platform-specific enumeration of key codes, and once it's decided what
the logical key press actually _is_, it hands off to the new functions
in terminal.c to generate the appropriate escape code.
Mostly this is intended to be a refactoring without functional change,
leaving the keyboard handling how it's always been. But in cases where
the Windows and GTK handlers were accidentally inconsistent, I've
fixed the inconsistency rather than carefully keeping both sides how
they were. Known consistency fixes:
- swapping the arrow keys between normal (ESC [ A) and application
(ESC O A) is now done by pressing Ctrl with them, and _not_ by
pressing Shift. That was how it was always supposed to work, and
how it's worked on GTK all along, but on Windows it's been done by
Shift as well since 2010, due to a bug at the call site of
format_arrow_key() introduced when I originally wrote that function.
- in Xterm function key mode plus application keypad mode, the /*-
keys on the numeric keypad now send ESC O {o,j,m} in place of ESC O
{Q,R,S}. That's how the Windows keyboard handler has worked all
along (it was a deliberate behaviour tweak for the Xterm-like
function key mode, because in that mode ESC O {Q,R,S} are generated
by F2-F4). But the GTK keyboard handler omitted that particular
special case and was still sending ESC O {Q,R,S} for those keys in
all application keypad modes.
- also in Xterm function key mode plus app keypad mode, we only
generates the app-keypad escape sequences if Num Lock is on; with
Num Lock off, the numeric keypad becomes arrow keys and
Home/End/etc, just as it would in non-app-keypad mode. Windows has
done this all along, but again, GTK lacked that special case.
This is another cleanup I felt a need for while I was doing
boolification. If you define a function or variable in one .c file and
declare it extern in another, then nothing will check you haven't got
the types of the two declarations mismatched - so when you're
_changing_ the type, it's a pain to make sure you've caught all the
copies of it.
It's better to put all those extern declarations in header files, so
that the declaration in the header is also in scope for the
definition. Then the compiler will complain if they don't match, which
is what I want.
My normal habit these days, in new code, is to treat int and bool as
_almost_ completely separate types. I'm still willing to use C's
implicit test for zero on an integer (e.g. 'if (!blob.len)' is fine,
no need to spell it out as blob.len != 0), but generally, if a
variable is going to be conceptually a boolean, I like to declare it
bool and assign to it using 'true' or 'false' rather than 0 or 1.
PuTTY is an exception, because it predates the C99 bool, and I've
stuck to its existing coding style even when adding new code to it.
But it's been annoying me more and more, so now that I've decided C99
bool is an acceptable thing to require from our toolchain in the first
place, here's a quite thorough trawl through the source doing
'boolification'. Many variables and function parameters are now typed
as bool rather than int; many assignments of 0 or 1 to those variables
are now spelled 'true' or 'false'.
I managed this thorough conversion with the help of a custom clang
plugin that I wrote to trawl the AST and apply heuristics to point out
where things might want changing. So I've even managed to do a decent
job on parts of the code I haven't looked at in years!
To make the plugin's work easier, I pushed platform front ends
generally in the direction of using standard 'bool' in preference to
platform-specific boolean types like Windows BOOL or GTK's gboolean;
I've left the platform booleans in places they _have_ to be for the
platform APIs to work right, but variables only used by my own code
have been converted wherever I found them.
In a few places there are int values that look very like booleans in
_most_ of the places they're used, but have a rarely-used third value,
or a distinction between different nonzero values that most users
don't care about. In these cases, I've _removed_ uses of 'true' and
'false' for the return values, to emphasise that there's something
more subtle going on than a simple boolean answer:
- the 'multisel' field in dialog.h's list box structure, for which
the GTK front end in particular recognises a difference between 1
and 2 but nearly everything else treats as boolean
- the 'urgent' parameter to plug_receive, where 1 vs 2 tells you
something about the specific location of the urgent pointer, but
most clients only care about 0 vs 'something nonzero'
- the return value of wc_match, where -1 indicates a syntax error in
the wildcard.
- the return values from SSH-1 RSA-key loading functions, which use
-1 for 'wrong passphrase' and 0 for all other failures (so any
caller which already knows it's not loading an _encrypted private_
key can treat them as boolean)
- term->esc_query, and the 'query' parameter in toggle_mode in
terminal.c, which _usually_ hold 0 for ESC[123h or 1 for ESC[?123h,
but can also hold -1 for some other intervening character that we
don't support.
In a few places there's an integer that I haven't turned into a bool
even though it really _can_ only take values 0 or 1 (and, as above,
tried to make the call sites consistent in not calling those values
true and false), on the grounds that I thought it would make it more
confusing to imply that the 0 value was in some sense 'negative' or
bad and the 1 positive or good:
- the return value of plug_accepting uses the POSIXish convention of
0=success and nonzero=error; I think if I made it bool then I'd
also want to reverse its sense, and that's a job for a separate
piece of work.
- the 'screen' parameter to lineptr() in terminal.c, where 0 and 1
represent the default and alternate screens. There's no obvious
reason why one of those should be considered 'true' or 'positive'
or 'success' - they're just indices - so I've left it as int.
ssh_scp_recv had particularly confusing semantics for its previous int
return value: its call sites used '<= 0' to check for error, but it
never actually returned a negative number, just 0 or 1. Now the
function and its call sites agree that it's a bool.
In a couple of places I've renamed variables called 'ret', because I
don't like that name any more - it's unclear whether it means the
return value (in preparation) for the _containing_ function or the
return value received from a subroutine call, and occasionally I've
accidentally used the same variable for both and introduced a bug. So
where one of those got in my way, I've renamed it to 'toret' or 'retd'
(the latter short for 'returned') in line with my usual modern
practice, but I haven't done a thorough job of finding all of them.
Finally, one amusing side effect of doing this is that I've had to
separate quite a few chained assignments. It used to be perfectly fine
to write 'a = b = c = TRUE' when a,b,c were int and TRUE was just a
the 'true' defined by stdbool.h, that idiom provokes a warning from
gcc: 'suggest parentheses around assignment used as truth value'!
I think this is the full set of things that ought logically to be
boolean.
One annoyance is that quite a few radio-button controls in config.c
address Conf fields that are now bool rather than int, which means
that the shared handler function can't just access them all with
conf_{get,set}_int. Rather than back out the rigorous separation of
int and bool in conf.c itself, I've just added a similar alternative
handler function for the bool-typed ones.
This commit includes <stdbool.h> from defs.h and deletes my
traditional definitions of TRUE and FALSE, but other than that, it's a
100% mechanical search-and-replace transforming all uses of TRUE and
FALSE into the C99-standardised lowercase spellings.
No actual types are changed in this commit; that will come next. This
is just getting the noise out of the way, so that subsequent commits
can have a higher proportion of signal.
After the recent Seat and LogContext revamps, _nearly_ all the
remaining uses of the type 'Frontend' were in terminal.c, which needs
all sorts of interactions with the GUI window the terminal lives in,
from the obvious (actually drawing text on the window, reading and
writing the clipboard) to the obscure (minimising, maximising and
moving the window in response to particular escape sequences).
All of those functions are now provided by an abstraction called
TermWin. The few remaining uses of Frontend after _that_ are internal
to a particular platform directory, so as to spread the implementation
of that particular kind of Frontend between multiple source files; so
I've renamed all of those so that they take a more specifically named
type that refers to the particular implementation rather than the
general abstraction.
So now the name 'Frontend' no longer exists in the code base at all,
and everywhere one used to be used, it's completely clear whether it
was operating in one of Frontend's three abstract roles (and if so,
which), or whether it was specific to a particular implementation.
Another type that's disappeared is 'Context', which used to be a
typedef defined to something different on each platform, describing
whatever short-lived resources were necessary to draw on the terminal
window: the front end would provide a ready-made one when calling
term_paint, and the terminal could request one with get_ctx/free_ctx
if it wanted to do proactive window updates. Now that drawing context
lives inside the TermWin itself, because there was never any need to
have two of those contexts live at the same time.
(Another minor API change is that the window-title functions - both
reading and writing - have had a missing 'const' added to their char *
parameters / return values.)
I don't expect this change to enable any particularly interesting new
functionality (in particular, I have no plans that need more than one
implementation of TermWin in the same application). But it completes
the tidying-up that began with the Seat and LogContext rework.
In the very old days, when PuTTY was new and computers were slow, I
tried to implement a feature where scrolling the window would be
implemented using a fast rectangle-copy GDI operation, rather than an
expensive character-by-character redraw of all the changed areas.
It never quite worked right, and I ended up conditioning it out on
Windows, and never even tried to implement it on GTK. It's now been
sitting around unused for so long that I think it's no longer worth
keeping in the code at all - if I tried to put it back in, it surely
wouldn't even compile, and would need rewriting from scratch anyway.
Disturbingly, it looks as if I _tried_ to re-enable it at one point,
in that there was a '#define OPTIMISE_IS_SCROLL 1' in putty.h - but
that never had any effect, because the macro name is misspelled. All
the #ifdefs are for 'OPTIMISE_SCROLL', without the 'IS'. So despite
appearances, it really _has_ been conditioned out all along!
That's more directly useful in uxpty.c (which is currently the only
actual client of the function), and also matches the data that SSH
clients send in "pty-req". Also, it makes that method behave more like
the GUI query function get_window_pixels used by terminal.c (with the
sole exception that unlike g_w_p it's allowed to return failure), so
it becomes even more trivial to implement in the GUI front ends.
This is a new vtable-based abstraction which is passed to a backend in
place of Frontend, and it implements only the subset of the Frontend
functions needed by a backend. (Many other Frontend functions still
exist, notably the wide range of things called by terminal.c providing
platform-independent operations on the GUI terminal window.)
The purpose of making it a vtable is that this opens up the
possibility of creating a backend as an internal implementation detail
of some other activity, by providing just that one backend with a
custom Seat that implements the methods differently.
For example, this refactoring should make it feasible to directly
implement an SSH proxy type, aka the 'jump host' feature supported by
OpenSSH, aka 'open a secondary SSH session in MAINCHAN_DIRECT_TCP
mode, and then expose the main channel of that as the Socket for the
primary connection'. (Which of course you can already do by spawning
'plink -nc' as a separate proxy process, but this would permit it in
the _same_ process without anything getting confused.)
I've centralised a full set of stub methods in misc.c for the new
abstraction, which allows me to get rid of several annoying stubs in
the previous code. Also, while I'm here, I've moved a lot of
duplicated modalfatalbox() type functions from application main
program files into wincons.c / uxcons.c, which I think saves
duplication overall. (A minor visible effect is that the prefixes on
those console-based fatal error messages will now be more consistent
between applications.)
This was used by ldisc to communicate back to the front end that a key
had been pressed (or rather, that a keypress had caused a nonzero
amount of session input data). Its only nontrivial implementation was
in gtkwin.c, which used that notification to implement the Unix GUI's
"close window on keypress, if the session was already over" policy.
(Which in turn is Unix-specific, because the rationale is that
sometimes X servers don't have a functioning window manager, so it's
useful to have a way of telling any application to close without using
WM-provided facilities like a close button.)
But gtkwin.c doesn't need to be told by the ldisc that a keypress
happened - it's the one _sending_ those keypresses to ldisc in the
first place! So I've thrown away the three stub implementations of
frontend_keypress, removed the call to it in ldisc.c, and replaced it
with calls in gtkwin.c at all the points during keypress handling
that call ldisc_send.
A visible effect is that pterm's close-on-keypress behaviour will now
only trigger on an actual (input-generating) _keypress_, and not on
other input generation such as a paste action. I think that's an
improvement.
LogContext is now the owner of the logevent() function that back ends
and so forth are constantly calling. Previously, logevent was owned by
the Frontend, which would store the message into its list for the GUI
Event Log dialog (or print it to standard error, or whatever) and then
pass it _back_ to LogContext to write to the currently open log file.
Now it's the other way round: LogContext gets the message from the
back end first, writes it to its log file if it feels so inclined, and
communicates it back to the front end.
This means that lots of parts of the back end system no longer need to
have a pointer to a full-on Frontend; the only thing they needed it
for was logging, so now they just have a LogContext (which many of
them had to have anyway, e.g. for logging SSH packets or session
traffic).
LogContext itself also doesn't get a full Frontend pointer any more:
it now talks back to the front end via a little vtable of its own
called LogPolicy, which contains the method that passes Event Log
entries through, the old askappend() function that decides whether to
truncate a pre-existing log file, and an emergency function for
printing an especially prominent message if the log file can't be
created. One minor nice effect of this is that console and GUI apps
can implement that last function subtly differently, so that Unix
console apps can write it with a plain \n instead of the \r\n
(harmless but inelegant) that the old centralised implementation
generated.
One other consequence of this is that the LogContext has to be
provided to backend_init() so that it's available to backends from the
instant of creation, rather than being provided via a separate API
call a couple of function calls later, because backends have typically
started doing things that need logging (like making network
connections) before the call to backend_provide_logctx. Fortunately,
there's no case in the whole code base where we don't already have
logctx by the time we make a backend (so I don't actually remember why
I ever delayed providing one). So that shortens the backend API by one
function, which is always nice.
While I'm tidying up, I've also moved the printf-style logeventf() and
the handy logevent_and_free() into logging.c, instead of having copies
of them scattered around other places. This has also let me remove
some stub functions from a couple of outlying applications like
Pageant. Finally, I've removed the pointless "_tag" at the end of
LogContext's official struct name.
