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

Message ID 20180221213741.19409-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Feb. 21, 2018, 9:37 p.m. UTC
  This is an updated version of a patch originally submitted here:

https://sourceware.org/ml/gdb-patches/2017-11/msg00556.html

This one updates the patch as requested and fixes a couple of typos in
the commit message from the second submission.

This changes to_fileio_readlink and target_fileio_readlink to return a
gdb::optional<std::sring>, and then fixes up the callers and
implementations.  This allows the removal of some cleanups.

Regression tested by the buildbot.

gdb/ChangeLog
2018-02-21  Tom Tromey  <tom@tromey.com>

	* linux-tdep.c (linux_info_proc): Update.
	* target.h (struct target_ops) <to_fileio_readlink>: Return
	optional<string>.
	(target_fileio_readlink): Return optional<string>.
	* remote.c (remote_hostio_readlink): Return optional<string>.
	* inf-child.c (inf_child_fileio_readlink): Return
	optional<string>.
	* target.c (target_fileio_readlink): Return optional<string>.
---
 gdb/ChangeLog    | 11 +++++++++++
 gdb/inf-child.c  | 12 ++++--------
 gdb/linux-nat.c  | 10 +++-------
 gdb/linux-tdep.c | 22 ++++++++--------------
 gdb/remote.c     | 12 +++++-------
 gdb/target.c     | 12 ++++++------
 gdb/target.h     | 13 ++++++-------
 7 files changed, 43 insertions(+), 49 deletions(-)
  

Comments

Tom Tromey March 7, 2018, 5:29 p.m. UTC | #1
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This is an updated version of a patch originally submitted here:
Tom> https://sourceware.org/ml/gdb-patches/2017-11/msg00556.html

Ping.

Tom
  
Simon Marchi March 7, 2018, 8:37 p.m. UTC | #2
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
  

Patch

diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 6875596f33..c7c45530b6 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -333,7 +333,7 @@  inf_child_fileio_unlink (struct target_ops *self,
 
 /* Implementation of to_fileio_readlink.  */
 
-static char *
+static gdb::optional<std::string>
 inf_child_fileio_readlink (struct target_ops *self,
 			   struct inferior *inf, const char *filename,
 			   int *target_errno)
@@ -343,22 +343,18 @@  inf_child_fileio_readlink (struct target_ops *self,
 #if defined (PATH_MAX)
   char buf[PATH_MAX];
   int len;
-  char *ret;
 
   len = readlink (filename, buf, sizeof buf);
   if (len < 0)
     {
       *target_errno = host_to_fileio_error (errno);
-      return NULL;
+      return {};
     }
 
-  ret = (char *) xmalloc (len + 1);
-  memcpy (ret, buf, len);
-  ret[len] = '\0';
-  return ret;
+  return std::string (buf, len);
 #else
   *target_errno = FILEIO_ENOSYS;
-  return NULL;
+  return {};
 #endif
 }
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index cc930c6334..1bbad7bacc 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4709,27 +4709,23 @@  linux_nat_fileio_open (struct target_ops *self,
 
 /* Implementation of to_fileio_readlink.  */
 
-static char *
+static gdb::optional<std::string>
 linux_nat_fileio_readlink (struct target_ops *self,
 			   struct inferior *inf, const char *filename,
 			   int *target_errno)
 {
   char buf[PATH_MAX];
   int len;
-  char *ret;
 
   len = linux_mntns_readlink (linux_nat_fileio_pid_of (inf),
 			      filename, buf, sizeof (buf));
   if (len < 0)
     {
       *target_errno = host_to_fileio_error (errno);
-      return NULL;
+      return {};
     }
 
-  ret = (char *) xmalloc (len + 1);
-  memcpy (ret, buf, len);
-  ret[len] = '\0';
-  return ret;
+  return std::string (buf, len);
 }
 
 /* Implementation of to_fileio_unlink.  */
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d54e56ce8d..b5bb16215d 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -764,26 +764,20 @@  linux_info_proc (struct gdbarch *gdbarch, const char *args,
   if (cwd_f)
     {
       xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", pid);
-      data = target_fileio_readlink (NULL, filename, &target_errno);
-      if (data)
-	{
-	  struct cleanup *cleanup = make_cleanup (xfree, data);
-          printf_filtered ("cwd = '%s'\n", data);
-	  do_cleanups (cleanup);
-	}
+      gdb::optional<std::string> contents
+	= target_fileio_readlink (NULL, filename, &target_errno);
+      if (contents.has_value ())
+	printf_filtered ("cwd = '%s'\n", contents->c_str ());
       else
 	warning (_("unable to read link '%s'"), filename);
     }
   if (exe_f)
     {
       xsnprintf (filename, sizeof filename, "/proc/%ld/exe", pid);
-      data = target_fileio_readlink (NULL, filename, &target_errno);
-      if (data)
-	{
-	  struct cleanup *cleanup = make_cleanup (xfree, data);
-          printf_filtered ("exe = '%s'\n", data);
-	  do_cleanups (cleanup);
-	}
+      gdb::optional<std::string> contents
+	= target_fileio_readlink (NULL, filename, &target_errno);
+      if (contents.has_value ())
+	printf_filtered ("exe = '%s'\n", contents->c_str ());
       else
 	warning (_("unable to read link '%s'"), filename);
     }
diff --git a/gdb/remote.c b/gdb/remote.c
index 15d6c5bdbf..7992a69e2a 100644
--- 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');
 
   read_len = remote_unescape_input ((gdb_byte *) attachment, attachment_len,
-				    (gdb_byte *) ret, len);
+				    (gdb_byte *) &ret[0], len);
   if (read_len != len)
     error (_("Readlink returned %d, but %d bytes."), len, read_len);
 
-  ret[len] = '\0';
   return ret;
 }
 
diff --git a/gdb/target.c b/gdb/target.c
index db7c09ba0f..ccce0fe73e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3059,7 +3059,7 @@  target_fileio_unlink (struct inferior *inf, const char *filename,
 
 /* See target.h.  */
 
-char *
+gdb::optional<std::string>
 target_fileio_readlink (struct inferior *inf, const char *filename,
 			int *target_errno)
 {
@@ -3069,22 +3069,22 @@  target_fileio_readlink (struct inferior *inf, const char *filename,
     {
       if (t->to_fileio_readlink != NULL)
 	{
-	  char *ret = t->to_fileio_readlink (t, inf, filename,
-					     target_errno);
+	  gdb::optional<std::string> ret
+	    = t->to_fileio_readlink (t, inf, filename, target_errno);
 
 	  if (targetdebug)
 	    fprintf_unfiltered (gdb_stdlog,
 				"target_fileio_readlink (%d,%s)"
 				" = %s (%d)\n",
 				inf == NULL ? 0 : inf->num,
-				filename, ret? ret : "(nil)",
-				ret? 0 : *target_errno);
+				filename, ret ? ret->c_str () : "(nil)",
+				ret ? 0 : *target_errno);
 	  return ret;
 	}
     }
 
   *target_errno = FILEIO_ENOSYS;
-  return NULL;
+  return {};
 }
 
 static void
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);
 
 
     /* Implement the "info proc" command.  */
@@ -2091,9 +2091,8 @@  extern int target_fileio_unlink (struct inferior *inf,
    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).  */
-extern char *target_fileio_readlink (struct inferior *inf,
-				     const char *filename,
-				     int *target_errno);
+extern gdb::optional<std::string> target_fileio_readlink
+    (struct inferior *inf, const char *filename, int *target_errno);
 
 /* Read target file FILENAME, in the filesystem as seen by INF.  If
    INF is NULL, use the filesystem seen by the debugger (GDB or, for