Re: [RFA v3] Return gdb::optional<std::string> from target_fileio_readlink

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2018-02-21 04:37 PM, Tom Tromey wrote:
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -11713,7 +11713,7 @@ remote_hostio_unlink (struct target_ops *self,
>  
>  /* Implementation of to_fileio_readlink.  */
>  
> -static char *
> +static gdb::optional<std::string>
>  remote_hostio_readlink (struct target_ops *self,
>  			struct inferior *inf, const char *filename,
>  			int *remote_errno)
> @@ -11724,10 +11724,9 @@ remote_hostio_readlink (struct target_ops *self,
>    int left = get_remote_packet_size ();
>    int len, attachment_len;
>    int read_len;
> -  char *ret;
>  
>    if (remote_hostio_set_filesystem (inf, remote_errno) != 0)
> -    return NULL;
> +    return {};
>  
>    remote_buffer_add_string (&p, &left, "vFile:readlink:");
>  
> @@ -11739,16 +11738,15 @@ remote_hostio_readlink (struct target_ops *self,
>  				    &attachment_len);
>  
>    if (len < 0)
> -    return NULL;
> +    return {};
>  
> -  ret = (char *) xmalloc (len + 1);
> +  std::string ret (len + 1, '\0');

I think it should be just "len" and not "len + 1" here.  The NULL byte is added by
the std::string on top of that length.  remote_unescape_input will only write len
bytes, so it won't touch that NULL byte.

> diff --git a/gdb/target.h b/gdb/target.h
> index 83cf48575f..05575df35f 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -935,10 +935,10 @@ struct target_ops
>         seen by the debugger (GDB or, for remote targets, the remote
>         stub).  Return a null-terminated string allocated via xmalloc,
>         or NULL if an error occurs (and set *TARGET_ERRNO).  */
> -    char *(*to_fileio_readlink) (struct target_ops *,
> -				 struct inferior *inf,
> -				 const char *filename,
> -				 int *target_errno);
> +    gdb::optional<std::string> (*to_fileio_readlink) (struct target_ops *,
> +						      struct inferior *inf,
> +						      const char *filename,
> +						      int *target_errno);

Can you update the comment above this?

LGTM with that fixed.

Simon