Re: [PATCH 5/6] Return target_xfer_status in to_xfer_partial

Pedro Alves
2014-02-06T14:44:48Z

On 01/31/2014 03:35 AM, Yao Qi wrote:
> This patch does the conversion of to_xfer_partial from
>
> LONGEST (*to_xfer_partial) (struct target_ops *ops,
> enum target_object object, const char *annex,
> gdb_byte *readbuf, const gdb_byte *writebuf,
> ULONGEST offset, ULONGEST len);
>
> to
>
> enum target_xfer_status (*to_xfer_partial) (struct target_ops *ops,
> enum target_object object, const char *annex,
> gdb_byte *readbuf, const gdb_byte *writebuf,
> ULONGEST offset, ULONGEST len, ULONGEST *xfered_len);
>
> It changes to_xfer_partial return the transfer status and the transfered
> length by *XFERED_LEN. Generally, the return status has three stats,
>
> - TARGET_XFER_OK,
> - TARGET_XFER_DONE,
> - TARGET_XFER_E_XXXX,

I was reading this and thinking "what's the difference
between OK and DONE again?" ...

> See the comments to them in 'enum target_xfer_status'. Note that
> Pedro suggested not name TARGET_XFER_DONE, as it is confusing,
> compared with "TARGET_XFER_OK". I still choose "done" because it has
> been used for a while.

Right. :-) This DONE status indicates that no further
transfer is possible. How about we call it TARGET_XFER_END or
TARGET_XFER_EOF ? EOF is pretty much universally understood
I think. I believe that'd be much clearer.

> size -= offset;
> if (size > len)
> size = len;
> @@ -714,7 +714,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
> return TARGET_XFER_E_IO;
> }
>
> - return size;
> + if (size == 0)
> + return TARGET_XFER_DONE;
> + else
> + {
> + *xfered_len = (ULONGEST) size;
> + return TARGET_XFER_OK;
> + }

If we move this size == 0 check above, we simplify
the code above too. Like:

if (size > len)
size = len;

+ if (size == 0)
+ return TARGET_XFER_DONE;

- if (size > 0
- && !bfd_get_section_contents (core_bfd, section, readbuf,
+ if (!bfd_get_section_contents (core_bfd, section, readbuf,
(file_ptr) offset, size))
{
warning (_("Couldn't read NT_AUXV note in core file."));
return TARGET_XFER_E_IO;
}

- return size;
+ *xfered_len = (ULONGEST) size;
+ return TARGET_XFER_OK;

> }
> return TARGET_XFER_E_IO;
>
> @@ -746,7 +752,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
> return TARGET_XFER_E_IO;
> }
>
> - return size;
> + if (size == 0)
> + return TARGET_XFER_DONE;
> + else
> + {
> + *xfered_len = (ULONGEST) size;
> + return TARGET_XFER_OK;
> + }
> }

Likewise.

>
> @@ -793,7 +822,7 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
>
> size = bfd_section_size (core_bfd, section);
> if (offset >= size)
> - return 0;
> + return TARGET_XFER_DONE;
> size -= offset;
> if (size > len)
> size = len;
> @@ -805,7 +834,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object,
> return TARGET_XFER_E_IO;
> }
>
> - return size;
> + if (size == 0)
> + return TARGET_XFER_DONE;
> + else
> + {
> + *xfered_len = (ULONGEST) size;
> + return TARGET_XFER_OK;
> + }

Ditto.

> -static LONGEST
> +static enum target_xfer_status
> linux_nat_xfer_osdata (struct target_ops *ops, enum target_object object,
> const char *annex, gdb_byte *readbuf,
> - const gdb_byte *writebuf, ULONGEST offset, ULONGEST len)
> + const gdb_byte *writebuf, ULONGEST offset, ULONGEST len,
> + ULONGEST *xfered_len)
> {
> gdb_assert (object == TARGET_OBJECT_OSDATA);
>
> - return linux_common_xfer_osdata (annex, readbuf, offset, len);
> + *xfered_len = linux_common_xfer_osdata (annex, readbuf, offset, len);
> + if (*xfered_len== 0)

Missing space.

> -static LONGEST
> +static enum target_xfer_status
> target_read_live_memory (enum target_object object,
> - ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len)
> + ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len,
> + ULONGEST *xfered_len)
> {
> - LONGEST ret;
> + enum target_xfer_status ret;
> struct cleanup *cleanup;
>
> /* Switch momentarily out of tfind mode so to access live memory.
> @@ -1326,8 +1327,8 @@ target_read_live_memory (enum target_object object,
> cleanup = make_cleanup_restore_traceframe_number ();
> set_traceframe_number (-1);
>
> - ret = target_read (current_target.beneath, object, NULL,
> - myaddr, memaddr, len);
> + ret = target_xfer_partial (current_target.beneath, object, NULL,
> + myaddr, NULL, memaddr, len, xfered_len);

This doesn't seem equivalent?

>
> if (readbuf)
> myaddr = readbuf;
> if (writebuf)
> myaddr = writebuf;
> - if (retval > 0 && myaddr != NULL)
> + if (retval == TARGET_XFER_OK && myaddr != NULL)
> {
> int i;
>
> fputs_unfiltered (", bytes =", gdb_stdlog);
> - for (i = 0; i < retval; i++)
> + for (i = 0; i < *xfered_len; i++)
> {
> if ((((intptr_t) &(myaddr[i])) & 0xf) == 0)
> {
> @@ -1763,12 +1786,19 @@ target_xfer_partial (struct target_ops *ops,
>
> fputc_unfiltered ('\n', gdb_stdlog);
> }
> +
> + /* Check implementations of to_xfer_partial update *XFERED_LEN
> + properly. Do assertion after printing debug messages, so that we
> + can find more clues on assertion failure from debugging messages. */
> + if (retval == TARGET_XFER_OK || retval == TARGET_XFER_E_UNAVAILABLE)
> + gdb_assert (*xfered_len > 0);

Indentation looks a little odd here.

> /* Wrappers to perform the full transfer. */
> @@ -2058,16 +2098,22 @@ target_read (struct target_ops *ops,
>
> while (xfered < len)
> {
> - LONGEST xfer = target_read_partial (ops, object, annex,
> - (gdb_byte *) buf + xfered,
> - offset + xfered, len - xfered);
> + ULONGEST xfered_len;
> + enum target_xfer_status status;
> +
> + status = target_read_partial (ops, object, annex,
> + (gdb_byte *) buf + xfered,
> + offset + xfered, len - xfered,
> + &xfered_len);
>
> /* Call an observer, notifying them of the xfer progress? */
> - if (xfer == 0)
> + if (status == TARGET_XFER_DONE)
> return xfered;
> - if (xfer < 0)
> + if (TARGET_XFER_STATUS_ERROR_P (status))
> return -1;
> - xfered += xfer;
> +
> + gdb_assert (status == TARGET_XFER_OK);
> + xfered += xfered_len;

I think you can avoid all these TARGET_XFER_SATUS_ERROR_P uses by
simply reversing the checks. E.g.,:

if (status == TARGET_XFER_DONE)
return xfered;
else if (status == TARGET_XFER_OK)
{
xfered += xfered_len;
QUIT;
}
else
return -1;

> }
> return len;
> @@ -2104,6 +2150,7 @@ read_whatever_is_readable (struct target_ops *ops,
> ULONGEST current_end = end;
> int forward;
> memory_read_result_s r;
> + ULONGEST xfered_len;
>
> /* If we previously failed to read 1 byte, nothing can be done here. */
> if (end - begin <= 1)
> @@ -2116,13 +2163,16 @@ read_whatever_is_readable (struct target_ops *ops,
> if not. This heuristic is meant to permit reading accessible memory
> at the boundary of accessible region. */
> if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
> - buf, begin, 1) == 1)
> + buf, begin, 1, &xfered_len) == TARGET_XFER_OK
> + && xfered_len == 1)

Didn't we assert that TARGET_XFER_OK transfers more than 0 ? Seems like
this "xfered_len == 1" check is redundant.

> {
> forward = 1;
> ++current_begin;
> }
> else if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL,
> - buf + (end-begin) - 1, end - 1, 1) == 1)
> + buf + (end-begin) - 1, end - 1, 1,
> + &xfered_len) == TARGET_XFER_OK
> + && xfered_len == 1)

Likewise.

>
> -/* Possible error codes returned by target_xfer_partial, etc. */
> +/* Possible values returned by target_xfer_partial, etc. */
>
> -enum target_xfer_error
> +enum target_xfer_status
> {
> + /* Some bytes are transferred. */
> + TARGET_XFER_OK = 1,
> +
> + /* No further transfer is possible. */
> + TARGET_XFER_DONE = 0,
> +
> /* Generic I/O error. Note that it's important that this is '-1',
> as we still have target_xfer-related code returning hardcoded
> '-1' on error. */
> @@ -219,9 +225,11 @@ enum target_xfer_error
> /* Keep list in sync with target_xfer_error_to_string. */
> };
>
> +#define TARGET_XFER_STATUS_ERROR_P(STATUS) (STATUS) < TARGET_XFER_DONE

Wrap in ()'s:

#define TARGET_XFER_STATUS_ERROR_P(STATUS) ((STATUS) < TARGET_XFER_DONE)

> /* Request that OPS transfer up to LEN 8-bit bytes of the target's
> OBJECT. The OFFSET, for a seekable object, specifies the
> @@ -518,10 +527,11 @@ struct target_ops
> starting point. The ANNEX can be used to provide additional
> data-specific information to the target.
>
> - Return the number of bytes actually transfered, zero when no
> - further transfer is possible, and a negative error code (really
> - an 'enum target_xfer_error' value) when the transfer is not
> - supported. Return of a positive value smaller than LEN does
> + Return the transferred status, error or OK (an
> + 'enum target_xfer_status' value). Save the number of bytes
> + actually transferred in *XFERED_LEN if transfer is successful
> + (TARGET_XFER_OK) or the requested data is unavailable
> + (TARGET_XFER_E_UNAVAILABLE). *XFERED_LEN smaller than LEN does

... Save the number of bytes
actually transferred in *XFERED_LEN if transfer is successful
(TARGET_XFER_OK) or the number of unavailable bytes if the requested data
is unavailable (TARGET_XFER_E_UNAVAILABLE). ...

Otherwise looks good to me.

Thanks a lot for doing this!

--
Pedro Alves