- Jan 29, 2020
-
-
Simon Tatham authored
A trawl through the code with -Wmissing-prototypes and -Wmissing-variable-declarations turned up a lot of things that should have been internal to a particular source file, but were accidentally global. Keep the namespace clean by making them all static. (Also, while I'm here, a couple of them were missing a 'const': the ONE and ZERO arrays in sshcrcda.c, and EMPTY_WINDOW_TITLE in terminal.c.)
-
- Nov 03, 2018
-
-
Simon Tatham authored
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'!
-
Simon Tatham authored
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.
-
- Oct 25, 2018
-
-
Simon Tatham authored
Colin Harrison points out that commit c31e3cd4 was less cautious than it should have been: when delete_callbacks_for_context nulls out the 'next' pointer in the new tail element of the callbacks list, it should only do so if there _is_ a new tail element. If the list has just become empty, that won't work very well!
-
- Oct 24, 2018
-
-
Simon Tatham authored
throttled_by_backlog is quite new, so it's not too surprising that I hadn't already noticed it wasn't initialised. But the failure to null out the final 'next' pointer in the callbacks list when it's rewritten as a result of delete_callbacks_for_context is a great deal older, so I'm a lot more puzzled about how it hasn't come up until now!
-
- Sep 19, 2018
-
-
Simon Tatham authored
This is another major source of unexplained 'void *' parameters throughout the code. In particular, the currently unused testback.c actually gave the wrong pointer type to its internal store of the frontend handle - it cast the input void * to a Terminal *, from which it got implicitly cast back again when calling from_backend, and nobody noticed. Now it uses the right type internally as well as externally.
-
- May 24, 2018
-
-
Simon Tatham authored
This seems to be a knock-on effect of my recent reworking of the SSH code to be based around queues and callbacks. The loop iteration function in uxsftp.c (ssh_sftp_do_select) would keep going round its select loop until something had happened on one of its file descriptors, and then return to the caller in the assumption that the resulting data might have triggered whatever condition the caller was waiting for - and if not, then the caller checks, finds nothing interesting has happened, and resumes looping with no harm done. But now, when something happens on an fd, it doesn't _synchronously_ trigger the follow-up condition PSFTP was waiting for (which, at startup time, happens to be back->sendok() starting to return TRUE). Instead, it schedules a callback, which will schedule a callback, which ... ends up setting that flag. But by that time, the loop function has already returned, the caller has found nothing interesting and resumed looping, and _now_ the interesting thing happens but it's too late because ssh_sftp_do_select will wait until the next file descriptor activity before it next returns. Solution: give run_toplevel_callbacks a return value which says whether it's actually done something, and if so, return immediately in case that was the droid the caller was looking for. As it were.
-
- May 18, 2018
-
-
Simon Tatham authored
This is a set of convenience wrappers around the existing toplevel callback function, which arranges to avoid scheduling a second call to a callback function if one is already in the queue. Just like the last few commits, this is a piece of infrastructure that nothing is yet using. But it will.
-
- Nov 26, 2017
-
-
Simon Tatham authored
This is used when you're about to destroy an object that is (potentially) the context parameter for some still-pending toplevel callback. It causes callbacks.c to go through its pending list and delete any callback records referring to that context parameter, so that when you destroy the object those callbacks aren't still waiting to cause stale-pointer dereferences.
-
- Sep 15, 2013
-
-
Simon Tatham authored
I temporarily applied it as a means of testing the revised event loops in r10040, and accidentally folded it into my final commit instead of backing it out. Ahem. [originally from svn r10042] [r10040 == 5c4ce2fa]
-
Simon Tatham authored
This change attempts to reinstate as a universal property something which was sporadically true of the ad-hockery that came before toplevel callbacks: that if there's a _very long_ queue of things to be done through the callback mechanism, the doing of them will be interleaved with re-checks of other event sources, which might (e.g.) cause a flag to be set which makes the next callback decide not to do anything after all. [originally from svn r10040]
-
- Aug 17, 2013
-
-
Simon Tatham authored
This is a little like schedule_timer, in that the callback you provide will be run from the top-level message loop of whatever application you're in; but unlike the timer mechanism, it will happen _immediately_. The aim is to provide a general way to avoid re-entrance of code, in cases where just _doing_ the thing you want done is liable to trigger a confusing recursive call to the function in which you came to the decision to do it; instead, you just request a top-level callback at the message loop's earliest convenience, and do it then. [originally from svn r10019]
-