Re: [rfc] Correct semantics of target_read_partial, add target_read_whole

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

Daniel Jacobowitz wrote:

> Originally, target_read_partial was supposed to read "however much it
> could manage to" and then higher level functions were supposed to handle
> the rest. But every existing implementation always reads enough data in
> its first call; the one remote protocol implementation did so by issuing
> as many packets as necessary, which defeated the point of the original
> design.
> 
> This patch adjusts the remote protocol layer not to do that.  It also
> promotes a useful function from auxv.c to target.c:
> 
> +/* Wrappers to perform a full read of unknown size.  OBJECT/ANNEX will
> +   be read using OPS.  The return value will be -1 if the transfer
> +   fails or is not supported; 0 if the object is empty; and the length
> +   of the object otherwise.  If a positive value is returned, a
> +   sufficiently large buffer will be allocated using xmalloc and
> +   returned in *BUF_P containing the contents of the object.
> +
> +   This method should be used for objects sufficiently small to store
> +   in a single xmalloced buffer, when no fixed bound on the object's
> +   size is known in advance.  Don't try to read TARGET_OBJECT_MEMORY
> +   through this function.  */
> +
> +extern LONGEST target_read_whole (struct target_ops *ops,
> +                                 enum target_object object,
> +                                 const char *annex, gdb_byte **buf_p);

Dan,

did you notice my post on gdb-devel about having two different methods of
reading method. The post subject was: "target memory read/write methods".
To quote:

  One way to read a memory is:

  - target_read_memory, which calls
    - xfer_using_stratum, which calls
      - target_xfer_partial (iterating over 'stratums'), which
        - goes to function pointer in target_ops

   This is the predominant method.

  Another way to read memory is:

  - get_target_memory{unsigned}, which calls:
    - target_read, which calls:
      - target_xfer_partial


Your patch add target_read_whole, that calls 'target_read', which until now
is used only by get_target_memory{unsigned}, which is used in 3 places in
entire gdb. In addition 'xfer_using_stratum' already has some code to
repeatedly call target_xfer_partial. I also might not understand what
stratums are but should not they be used in all cases? Otherwise, behaviour
of target_read_while for 'object = TARGET_OBJECT_MEMORY' will be different
from the behaviour of 'target_read_memory'.

I think that either:
1. There are way too many code paths for reading
2. There are way too few comments.

Is seems to me that the right solution would be to have target_read_whole
directly call xfer_using_stratum.

Also, how you target_read_whole work when   

  target_xfer_partial_p ()

return false? The 'target_read' function does not check it and
'target_xfer_partial' will just assert.

Should 'target_read_whole' check for target_xfer_partial_p and fail is its
false?

- Volodya