Re: [PATCH] Cleanup target memory reading

Vladimir Prus
2006-06-29T09:34:08Z

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