Skip to content
Snippets Groups Projects
  1. Jan 29, 2020
    • Simon Tatham's avatar
      Add lots of missing 'static' keywords. · 8d747d80
      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.)
      8d747d80
  2. Nov 03, 2018
    • Simon Tatham's avatar
      Convert a lot of 'int' variables to 'bool'. · 3214563d
      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'!
      3214563d
    • Simon Tatham's avatar
      Adopt C99 <stdbool.h>'s true/false. · a6f1709c
      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.
      a6f1709c
  3. Oct 25, 2018
    • Simon Tatham's avatar
      Fix a newly introduced segfault in callback.c. · 6714fcdd
      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!
      6714fcdd
  4. Oct 24, 2018
    • Simon Tatham's avatar
      Fix a couple of uninitialised variables. · c31e3cd4
      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!
      c31e3cd4
  5. Sep 19, 2018
    • Simon Tatham's avatar
      Introduce a typedef for frontend handles. · 8dfb2a11
      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.
      8dfb2a11
  6. May 24, 2018
    • Simon Tatham's avatar
      Fix startup hang in Unix file transfer tools. · b8c4d042
      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.
      b8c4d042
  7. May 18, 2018
    • Simon Tatham's avatar
      Add a concept of 'idempotent callback'. · 2ee07f8c
      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.
      2ee07f8c
  8. Nov 26, 2017
    • Simon Tatham's avatar
      New facility for removing pending toplevel callbacks. · afa9734b
      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.
      afa9734b
  9. Sep 15, 2013
    • Simon Tatham's avatar
      Oops! Remove a tight-looping diagnostic. · a3d069d2
      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]
      a3d069d2
    • Simon Tatham's avatar
      Only run one toplevel callback per event loop iteration. · 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]
      5c4ce2fa
  10. Aug 17, 2013
    • Simon Tatham's avatar
      Add a general way to request an immediate top-level callback. · 75c79e31
      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]
      75c79e31
Loading