Re: [PATCH 7/8] Adjust read_value_memory to use to_xfer_partial

[Pedro Alves <palves at redhat dot com>]

On 02/12/2014 06:05 AM, Yao Qi wrote:
> As the new to_xfer_partial implementations are done in ctf and tfile
> targets

I think we'll want to move the traceframe_available_memory
code from memory_xfer_partial_1 down to the targets too
(at some point).

> -	  if (unavail != memaddr + length)
> -	    mark_value_bytes_unavailable (val,
> -					  embedded_offset + unavail - memaddr,
> -					  (memaddr + length) - unavail);
> +      if (status == TARGET_XFER_EOF)
> +	memory_error (TARGET_XFER_E_IO, memaddr + xfered);
> +      else if (status == TARGET_XFER_E_UNAVAILABLE)
> +	mark_value_bytes_unavailable (val, embedded_offset + xfered,
> +				      xfered_len);
> +      else if (TARGET_XFER_STATUS_ERROR_P (status))
> +	memory_error (status, memaddr + xfered);

I maintain that using TARGET_XFER_STATUS_ERROR_P tends to
be unnecessary, and actually leads to fragile code.   It doesn't
really help here.  A newer status added in the future that is
not an error will pass by here unhandled.  Handle TARGET_XFER_OK
explicitly instead, like this:

      if (status == TARGET_XFER_EOF)
	/* nothing */;
      else if (status == TARGET_XFER_E_UNAVAILABLE)
	mark_value_bytes_unavailable (val, embedded_offset + xfered,
				      xfered_len);
      else if (status == TARGET_XFER_EOF)
	memory_error (TARGET_XFER_E_IO, memaddr + xfered);
      else
	memory_error (status, memaddr + xfered);

      xfered += xfered_len;
      QUIT;
    }

and now all future unhandled statuses fall through to
memory_error.

(Hmm, the _E_ in TARGET_XFER_E_UNAVAILABLE is now starting
to look a little odd and confusing to me.  E.g., it's not
really an error in this case.  I'm pondering renaming it to
TARGET_XFER_UNAVAILABLE.)

Otherwise looks good, thanks!

-- 
Pedro Alves