[Bug 30292] openssl backend does not handle fatal errors during async writes correctly

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=30292

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |r-

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2010-09-21 03:23:19 PDT ---
The commit message doesn't really describe the bug or the fix very well. How
about something like:

  Don't retry indefinitely on failed OpenSSL writes

  Previously, the response to any partial write was to try again later, even if
the error returned indicated that it would never succeed. Instead, we should
only retry later if WOULD_BLOCK is returned (indicating only that the write
buffer is full, as opposed to an actual error).

  This error was only in the OpenSSL backend; gnutls does not require us to
implement this logic at all.

+      else
+        {
+          /* should never happen - GIO bug? */
+          g_warning ("Incomplete GIO operation did not return an error");
+        }

Company on #gnome-hackers suggests that a partial write with no error is
reasonable behaviour, as does man 2 write. And the documentation for
g_output_stream_write_async() suggests the same:

> On success, the number of bytes written will be passed to the
> callback. It is not an error if this is not the same as the requested
> size, as it can happen e.g. on a partial I/O error, but generally we
> try to write as many bytes as requested.

So it seems like this should indeed fall through to the same retry path as
WOULD_BLOCK, but that a warning is overly alarmist.

+          /* EWOULDBLOCK - expected for async operations. Anything else *
+           * is actually a real error an indicates we should abort      */

                                   and ^^

I'm kind of allergic to this shape of comment block, preferring

  /* Foo bar
   * and also baz
   */

but that could just be me. That style seems more common in these codebases,
though.

Maybe the whole block for written != buffered could be made easier to follow
with a bit of reordering. With your patch, the logic is:

 • check error, and frob it a bit
 • update the buffer
 • and then check the error again

It would be clearer if the buffer update came before all the error-related
code. 

+        wocky_tls_session_try_operation (session, WOCKY_TLS_OP_WRITE);

Supposedly this invocation makes us bail out. It's not clear to me how this
works. But maybe this is me being unfamiliar with the code.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.