Re: [PATCH] Cleanup target memory reading

[Vladimir Prus <ghost@cs.msu.su>]

Daniel Jacobowitz wrote:

>> Shouldn't we "fix" target_read() such that it uses
>> target_read_partial() instead?

> It already does.  It's just not an operation that is often useful.
> 
> We've got four, if you include my pending (pending rewrite, that is)
> patch:
> 
>   - target_read_partial.  This is now an implementation detail.  It
> reads some bytes.  Who knows how many.
>   - target_read.  It reads a fixed, requested number of bytes.
>   - xfer_using_stratum.  Similar, but allow the request to be split and
> parts handled by different strata.  This is always the right choice for
> memory, at least for consistency.
>   - target_read_object_alloc, which is my current thinking of a name
> for what was target_read_whole in my patch.  This is the basically
> always appropriate choice to fetch an "object" other than memory, since
> we don't have any way to find out what size it is otherwise, so we
> can't request a particular size.
> 
> I don't want that last one to use the second one, because it's prone to
> doing this:
> 
>   - target_read_object_alloc
>     - target_read 512 bytes
>       - target_read_partial gets 500
>       - Second target_read_partial for 12 bytes
>     - Another target_read
> 
> And we'd much rather group them in larger batches than twelve byte
> reads.
> 
> I think Vlad's basically garbage collecting target_read, since #3 and
> #4 from my list cover all the needs we've got right now.  Does this
> seem reasonable?  I think we've got too many ways to do this, and could
> do with fewer.

Yea, garbage collecting is precisely what I do here. The target_read is not
very useful to reading memory, since it's ignores strata, and it's not very
convenient for reading objects, since your upcoming
target_read_object_alloc does not require a caller to allocate buffer of
unknown size in advance.

If fact, I wanted to kill target_write as well, but won't do this until
either you and I implement qXfer write operation.

> I'd even suggest a pass over the other memory reading functions,
> pruning.  For instance, the ones that take a frame.  We know why they
> were added, and it's in theory a good plan to associate memory reads
> with frames, but it's not going to be completed any time in the
> foreseeable future.  We've got lots of what the GCC folks called an
> incomplete transition when they were ranting about internal
> cleanliness.

I can do a second look, it this seems reasonable.

- Volodya