[MERGE] chunked sftp readv

[Martin Pool]

On Fri, Sep 5, 2008 at 1:12 AM, John Arbash Meinel
<john at arbash-meinel.com> wrote:
>> Should this subclass a TransportError or something?
>>
>> Would it be useful to show the particular values in the error?  In
>> general I think it's good to capture the most information possible at
>> the time the error occurs but here the raw numbers may not help much
>> without more investigation.
>>
>> You're missing a ')'.  Use this one --> )  :-)
>
> We can. It never happens, and it was mostly making clear that the api
> forbids it. In a way, we could just get away with an "AssertionError".
> The readv() api wasn't specific about whether overlapping was allowed,
> but we've never needed it, and it allows us to consume data as it comes
> in, rather than worrying that it might be needed for another range.

If it should never happen and indicates a bug in the calling code, and
therefore no one wants to catch it, I think a plain AssertionError
would be fine.  I have a vague rule in mind that we should only add a
new exception class if someone could reasonably catch it.

>> +
>> +    def assertGetRequests(self, expected_requests, offsets):
>>
>> I think generally our style is to call things that actually do the test
>> checkGetRequests (or check_get_requests), because it's more than just an
>> assertion.
>
> Really? I've been using assertFoo for this for a while, I can change if
> that is truly the case.

I should have said that it's not done very consistently.  But I think
it is clearer to distinguish between calls that are just assertions
about values (if predicate: raise) and those that actually run the
test.  If they're testing a pure function then the distinction is
fuzzy.

>> +    def _coalesce_offsets(offsets, limit=0, fudge_factor=0, max_size=0,
>> +                          allow_overlap=False):
>>
>> I think this would previously allow you to pass in overlapping ranges
>> and as one effect it would clear them up.  That may never have been used
>> but does it particularly need to be disallowed at this level?  (I can
>> see how it makes sense not to require transports to handle it.)  If you
>> do want to keep it, could you say so in the docstring.
>>
>> The code change itself seems correct afaics.
>>
>
> It would collapse overlapping ranges, but the higher levels probably
> didn't support it. Specifically, the really old ones that just buffered
> everything would be fine (I think HTTP falls into this), but newer ones
> that deallocate the temporary buffers will fail.
>
> I mostly wanted to assert this in the _sftp_readv and other such helpers
> (RemoteTransport._readv could use this as well). And it seemed nice to
> put it in a common place between all helpers.

OK, fine with me then.

-- 
Martin <http://launchpad.net/~mbp/>