Re: [RFA] gdbserver/win32-low.c: Check Read/WriteProcessMemory return value (followup to [RFA] windows-nat.c: Handle ERROR_PARTIAL_COPY in windows_xfer_memory function)

[Pedro Alves <palves@redhat.com>]

On 09/02/2013 03:00 PM, Pierre Muller wrote:
>>>   What about this patch,
>>> it still does not allow to really return the number of bytes read or
>>> written,
>>> but at least it checks correctly if the API calls succeeded.
>>
>> No, as long as the read_memory/write_memory interfaces do not
>> support partial transfers, we should only return true if the
>> all of LEN was transferred.  Otherwise, things like:
>>
>> static int
>> gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>> {
>> ...
>>     {
>>       res = read_inferior_memory (memaddr, myaddr, len);
>>       done_accessing_memory ();
>>
>>       return res == 0 ? len : -1;
>>     }
>> }
>>
>> will behave incorrectly in the ERROR_PARTIAL_COPY scenario...
> 
>   This is still done in win32_{read/write}_inferior_memory which are the two
> only callers of the static child_xfer_memory function in win32-low.c 

>   Thus the aim was to narrow the behavior gap between
> windows-nat.c windows_xfer_memory function 
> and the win32-low.c child_xfer_memory function,
> without (for now) changing anything to the beghavior of gdbserver,
> as guaranteed by the 
> static int
> win32_write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
>                              int len)
> {
>   return child_xfer_memory (memaddr, (char *) myaddr, len, 1, 0) != len;
> }
> 
> code...
> 
> The only thing I changed is that child_xfer_memory returns the correct
> amount of read/written memory or -1 if an error, other than
> ERRO_PARTIAL_COPY, occurred.
> Thus I think that your answer is missing the intermediate
> win32_{read/write}_inferior_memory level.
> 

Ah, indeed.


Why the different styles in gdb's and gdbserver patches, though?

gdb:

> +  if (!success && lasterror == ERROR_PARTIAL_COPY && done > 0)
> +    return done;
> +  else
> +    return success ? done : TARGET_XFER_E_IO;

gdbserver:

> +  if (success)
> +    return done;
> +  else
> +    {
> +      if (lasterror == ERROR_PARTIAL_COPY && done > 0)
> +	return done;
> +      else
> +	return -1;
>      }

We should be able to compare the functions and see at
a glance they are almost duplicates.  With the different
styles, it's not immediately obvious.  Can you make the
gdbserver code look like gdb's?

Thanks,
-- 
Pedro Alves