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

Simon Marchi
2018-03-07T20:37:59Z

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