Re: [RFA] i386-tdep.c, check target_read_memory for error.

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Sun, 06 Mar 2011 19:42:34 +0100, Michael Snyder wrote:
> Jan Kratochvil wrote:
> >On Fri, 04 Mar 2011 22:37:52 +0100, Michael Snyder wrote:
> >>Call error if target_read_memory fails.
> >[...]
> >>-  target_read_memory (pc, &op, 1);
> >>+  if (target_read_memory (pc, &op, 1))
> >>+    error (_("Couldn't read memory at pc (%s)"), +	   paddress
> >>(gdbarch, pc));
> >
> >There is the function `read_memory' for such purpose.
> 
> I don't understand the objection.  target_read_memory may fail and
> return an error code.  Coverity reports that the return value is checked
> in 78 out of 97 calling instances.  So what's wrong with checking it now?

I was suggesting that instead of the code
  if (target_read_memory (pc, &op, 1))
    error (_("Couldn't read memory at pc (%s)"), paddress (gdbarch, pc));

One can write a shorter code with the same effect:
  read_memory (pc, &op, 1);

I did not do a real patch review and I also did not write any notice about any
review/approval.

Mark Kettenis correctly noticed that the introduced errors (either by error or
through read_memory) in some of the cases are wrong / cause regressions.

Just if in some cases the error is appropriate (I do not say in which specific
cases, if any) I was suggesting calling `read_memory' is more suitable than
the explicit `target_read_memory'+`error' calls.


Thanks,
Jan