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

Message ID 20180221213741.19409-1-tom@tromey.com
State New
Headers show
Series
  • [RFA,v3] Return gdb::optional<std::string> from target_fileio_readlink
Related show

Commit Message

Tom Tromey Feb. 21, 2018, 9:37 p.m.
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(-)

-- 
2.13.6

Comments

Tom Tromey March 7, 2018, 5:29 p.m. | #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. | #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
Tom Tromey March 7, 2018, 10:44 p.m. | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:


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

Makes sense.

Simon> Can you update the comment above this?

Simon> LGTM with that fixed.

Here's what I'm checking in.

Tom

commit e0d3522b888033febd153a4a7d3b7c5c13641f09
Author: Tom Tromey <tom@tromey.com>
Date:   Wed Nov 22 23:37:38 2017 -0700

    Return gdb::optional<std::string> from target_fileio_readlink
    
    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-03-07  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>.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 928dbab31f..49b91e1e15 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2018-03-07  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>.
+
 2018-03-07  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* regcache.c (cooked_read_test): Add riscv to the list of
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..134a97eed9 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, '\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 197e7ff542..2b97118217 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 de562139c7..fd6c30b983 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -933,12 +933,12 @@ struct target_ops
     /* Read value of symbolic link FILENAME on the target, in the
        filesystem as seen by INF.  If INF is NULL, use the filesystem
        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);
+       stub).  Return a string, or an empty optional if an error
+       occurs (and set *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.  */
@@ -2095,9 +2095,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

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