Code review comment for lp:~spiv/bzr/no_until_no_eintr

Martin [gz] wrote:
> Interesting on the encode tuple unicode issue, I didn't see any issues there
> in testing. Nit, you've got the ISO 8601 basic format wrong, it's big endian.

Heh, fixed :)

> Having thought about this a bit more, I'm not mad about a bzrlib.smart.medium
> focused change. The module is already wrappers around wrappers around
> wrappers, and it's too hard to work out what's going on. Case in point - as
> far as I can tell, SmartSSHClientMedium still uses get_filelike_channels which
> uses an unsafe wrapper for sockets pre-2.7 in _ParamikoSSHConnection and
> non-EINTR-protected pipes in SSHSubprocess. This means we're not covering
> codepaths like the one bug 341535 highlighted.

That's true. However I've just sent this patch to PQM anyway because I think
it's still a clear improvement. Here's my reasoning:

  - I think regressing 341535 is better than risking silent corruption of HPSS data
    (and certainly better than a confusing error caused by corrupted HPSS data),
  - Our SIGWINCH handler uses siginterrupt to avoid triggering EINTR, and
  - bzrlib's only other signal handler is SIGQUIT, which is a developer tool not
    a production feature.

I'll reopen 341535, of course.

Thanks for another thoughtful review!