[PATCH RFA #2] Retry open()s in procfs.c

[PATCH RFA #2] Retry open()s in procfs.c

[Kevin Buettner]

I request approval for committing the patch below.

This patch supercedes the patch I submitted in:

    http://sources.redhat.com/ml/gdb-patches/2001-04/msg00133.html

In addition to handling the original problem that I was seeing on AIX5
(which involved a race between GDB and the kernel in opening/creating
/proc/PID/ctl), this patch handles other conditions in which it may be
desirable to retry an open() in procfs.c.  In particular, I hope that
it will also fix the problem reported by Damien Kick in:

    http://sources.redhat.com/ml/gdb/2001-04/msg00078.html

(Damien, I would appreciate it if you'd give this patch a try and
let me know if it fixes the problem that you reported.)

I've tested these changes on AIX5 and it does gracefully handle the
race condition noted above.  (I added a temporary printf() just before
the sleep() to verify that the ``else'' clause does indeed get hit and
operates as expected.)  I've also run the GDB testsuite on both
i586-sco-sysv5uw7.1.1 and i386-pc-solaris2.8 and see no regressions.

Okay to commit?

	* procfs.c (open_with_retry): New function.
	(open_procinfo_files, load_syscalls, proc_iterate_over_mappings)
	(proc_get_LDT_entry): Call open_with_retry() instead of open().

Index: procfs.c
===================================================================
RCS file: /cvs/src/src/gdb/procfs.c,v
retrieving revision 1.26
diff -u -p -r1.26 procfs.c
--- procfs.c	2001/03/27 02:01:11	1.26
+++ procfs.c	2001/04/14 01:37:56
@@ -458,6 +458,40 @@ find_procinfo_or_die (int pid, int tid)
   return pi;
 }
 
+/* open_with_retry() is a wrapper for open().  The appropriate
+   open() call is attempted; if unsuccessful, it will be retried as
+   many times as needed for the EAGAIN and EINTR conditions.
+   
+   For other conditions, open_with_retry() will retry the open() a
+   limited number of times.  In addition, a short sleep is imposed
+   prior to retrying the open().  The reason for this sleep is to give
+   the kernel a chance to catch up and create the file in question in
+   the event that GDB "wins" the race to open a file before the kernel
+   has created it.  */
+   
+static int
+open_with_retry (const char *pathname, int flags)
+{
+  int retries_remaining, status;
+
+  retries_remaining = 2;
+
+  while (1)
+    {
+      status = open (pathname, flags);
+
+      if (status >= 0 || retries_remaining == 0)
+	break;
+      else if (errno != EINTR && errno != EAGAIN)
+	{
+	  retries_remaining--;
+	  sleep (1);
+	}
+    }
+
+  return status;
+}
+
 /*
  * Function: open_procinfo_files
  *
@@ -543,7 +577,7 @@ open_procinfo_files (procinfo *pi, int w
       strcat (tmp, "/lwpctl");
     else
       strcat (tmp, "/ctl");
-    fd = open (tmp, O_WRONLY);
+    fd = open_with_retry (tmp, O_WRONLY);
     if (fd <= 0)
       return 0;		/* fail */
     pi->ctl_fd = fd;
@@ -552,7 +586,7 @@ open_procinfo_files (procinfo *pi, int w
     if (pi->tid)
       return 0;		/* there is no 'as' file descriptor for an lwp */
     strcat (tmp, "/as");
-    fd = open (tmp, O_RDWR);
+    fd = open_with_retry (tmp, O_RDWR);
     if (fd <= 0)
       return 0;		/* fail */
     pi->as_fd = fd;
@@ -562,7 +596,7 @@ open_procinfo_files (procinfo *pi, int w
       strcat (tmp, "/lwpstatus");
     else
       strcat (tmp, "/status");
-    fd = open (tmp, O_RDONLY);
+    fd = open_with_retry (tmp, O_RDONLY);
     if (fd <= 0)
       return 0;		/* fail */
     pi->status_fd = fd;
@@ -586,12 +620,13 @@ open_procinfo_files (procinfo *pi, int w
 
 
 #ifdef PIOCTSTATUS	/* OSF */
-  if ((fd = open (pi->pathname, O_RDWR)) == 0) /* Only one FD; just open it. */
+  /* Only one FD; just open it. */
+  if ((fd = open_with_retry (pi->pathname, O_RDWR)) == 0)
     return 0;
 #else			/* Sol 2.5, Irix, other? */
   if (pi->tid == 0)	/* Master procinfo for the process */
     {
-      fd = open (pi->pathname, O_RDWR);
+      fd = open_with_retry (pi->pathname, O_RDWR);
       if (fd <= 0)
 	return 0;	/* fail */
     }
@@ -843,7 +878,7 @@ load_syscalls (procinfo *pi)
 
   /* Open the file descriptor for the sysent file */
   sprintf (pathname, "/proc/%d/sysent", pi->pid);
-  sysent_fd = open (pathname, O_RDONLY);
+  sysent_fd = open_with_retry (pathname, O_RDONLY);
   if (sysent_fd < 0)
     {
       error ("load_syscalls: Can't open /proc/%d/sysent", pi->pid);
@@ -2874,7 +2909,7 @@ proc_iterate_over_mappings (int (*func) 
 #ifdef NEW_PROC_API
   /* Open map fd.  */
   sprintf (pathname, "/proc/%d/map", pi->pid);
-  if ((map_fd = open (pathname, O_RDONLY)) < 0)
+  if ((map_fd = open_with_retry (pathname, O_RDONLY)) < 0)
     proc_error (pi, "proc_iterate_over_mappings (open)", __LINE__);
 
   /* Make sure it gets closed again.  */
@@ -2903,7 +2938,7 @@ proc_iterate_over_mappings (int (*func) 
 	{
 	  sprintf (name, "/proc/%d/object/%s", pi->pid, map->pr_mapname);
 	  /* Note: caller's responsibility to close this fd!  */
-	  fd = open (name, O_RDONLY);
+	  fd = open_with_retry (name, O_RDONLY);
 	  /* Note: we don't test the above call for failure;
 	     we just pass the FD on as given.  Sometimes there is 
 	     no file, so the ioctl may return failure, but that's
@@ -2982,7 +3017,7 @@ proc_get_LDT_entry (procinfo *pi, int ke
 
   /* Open the file descriptor for the LDT table.  */
   sprintf (pathname, "/proc/%d/ldt", pi->pid);
-  if ((fd = open (pathname, O_RDONLY)) < 0)
+  if ((fd = open_with_retry (pathname, O_RDONLY)) < 0)
     {
       proc_warn (pi, "proc_get_LDT_entry (open)", __LINE__);
       return NULL;

Re: [PATCH RFA #2] Retry open()s in procfs.c

[Michael Snyder]

Kevin Buettner wrote:
> 
> I request approval for committing the patch below.

Yes, this looks great.  Please put it in.

> 
> This patch supercedes the patch I submitted in:
> 
>     http://sources.redhat.com/ml/gdb-patches/2001-04/msg00133.html
> 
> In addition to handling the original problem that I was seeing on AIX5
> (which involved a race between GDB and the kernel in opening/creating
> /proc/PID/ctl), this patch handles other conditions in which it may be
> desirable to retry an open() in procfs.c.  In particular, I hope that
> it will also fix the problem reported by Damien Kick in:
> 
>     http://sources.redhat.com/ml/gdb/2001-04/msg00078.html
> 
> (Damien, I would appreciate it if you'd give this patch a try and
> let me know if it fixes the problem that you reported.)
> 
> I've tested these changes on AIX5 and it does gracefully handle the
> race condition noted above.  (I added a temporary printf() just before
> the sleep() to verify that the ``else'' clause does indeed get hit and
> operates as expected.)  I've also run the GDB testsuite on both
> i586-sco-sysv5uw7.1.1 and i386-pc-solaris2.8 and see no regressions.
> 
> Okay to commit?
> 
>         * procfs.c (open_with_retry): New function.
>         (open_procinfo_files, load_syscalls, proc_iterate_over_mappings)
>         (proc_get_LDT_entry): Call open_with_retry() instead of open().
> 
> Index: procfs.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/procfs.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 procfs.c
> --- procfs.c    2001/03/27 02:01:11     1.26
> +++ procfs.c    2001/04/14 01:37:56
> @@ -458,6 +458,40 @@ find_procinfo_or_die (int pid, int tid)
>    return pi;
>  }
> 
> +/* open_with_retry() is a wrapper for open().  The appropriate
> +   open() call is attempted; if unsuccessful, it will be retried as
> +   many times as needed for the EAGAIN and EINTR conditions.
> +
> +   For other conditions, open_with_retry() will retry the open() a
> +   limited number of times.  In addition, a short sleep is imposed
> +   prior to retrying the open().  The reason for this sleep is to give
> +   the kernel a chance to catch up and create the file in question in
> +   the event that GDB "wins" the race to open a file before the kernel
> +   has created it.  */
> +
> +static int
> +open_with_retry (const char *pathname, int flags)
> +{
> +  int retries_remaining, status;
> +
> +  retries_remaining = 2;
> +
> +  while (1)
> +    {
> +      status = open (pathname, flags);
> +
> +      if (status >= 0 || retries_remaining == 0)
> +       break;
> +      else if (errno != EINTR && errno != EAGAIN)
> +       {
> +         retries_remaining--;
> +         sleep (1);
> +       }
> +    }
> +
> +  return status;
> +}
> +
>  /*
>   * Function: open_procinfo_files
>   *
> @@ -543,7 +577,7 @@ open_procinfo_files (procinfo *pi, int w
>        strcat (tmp, "/lwpctl");
>      else
>        strcat (tmp, "/ctl");
> -    fd = open (tmp, O_WRONLY);
> +    fd = open_with_retry (tmp, O_WRONLY);
>      if (fd <= 0)
>        return 0;                /* fail */
>      pi->ctl_fd = fd;
> @@ -552,7 +586,7 @@ open_procinfo_files (procinfo *pi, int w
>      if (pi->tid)
>        return 0;                /* there is no 'as' file descriptor for an lwp */
>      strcat (tmp, "/as");
> -    fd = open (tmp, O_RDWR);
> +    fd = open_with_retry (tmp, O_RDWR);
>      if (fd <= 0)
>        return 0;                /* fail */
>      pi->as_fd = fd;
> @@ -562,7 +596,7 @@ open_procinfo_files (procinfo *pi, int w
>        strcat (tmp, "/lwpstatus");
>      else
>        strcat (tmp, "/status");
> -    fd = open (tmp, O_RDONLY);
> +    fd = open_with_retry (tmp, O_RDONLY);
>      if (fd <= 0)
>        return 0;                /* fail */
>      pi->status_fd = fd;
> @@ -586,12 +620,13 @@ open_procinfo_files (procinfo *pi, int w
> 
> 
>  #ifdef PIOCTSTATUS     /* OSF */
> -  if ((fd = open (pi->pathname, O_RDWR)) == 0) /* Only one FD; just open it. */
> +  /* Only one FD; just open it. */
> +  if ((fd = open_with_retry (pi->pathname, O_RDWR)) == 0)
>      return 0;
>  #else                  /* Sol 2.5, Irix, other? */
>    if (pi->tid == 0)    /* Master procinfo for the process */
>      {
> -      fd = open (pi->pathname, O_RDWR);
> +      fd = open_with_retry (pi->pathname, O_RDWR);
>        if (fd <= 0)
>         return 0;       /* fail */
>      }
> @@ -843,7 +878,7 @@ load_syscalls (procinfo *pi)
> 
>    /* Open the file descriptor for the sysent file */
>    sprintf (pathname, "/proc/%d/sysent", pi->pid);
> -  sysent_fd = open (pathname, O_RDONLY);
> +  sysent_fd = open_with_retry (pathname, O_RDONLY);
>    if (sysent_fd < 0)
>      {
>        error ("load_syscalls: Can't open /proc/%d/sysent", pi->pid);
> @@ -2874,7 +2909,7 @@ proc_iterate_over_mappings (int (*func)
>  #ifdef NEW_PROC_API
>    /* Open map fd.  */
>    sprintf (pathname, "/proc/%d/map", pi->pid);
> -  if ((map_fd = open (pathname, O_RDONLY)) < 0)
> +  if ((map_fd = open_with_retry (pathname, O_RDONLY)) < 0)
>      proc_error (pi, "proc_iterate_over_mappings (open)", __LINE__);
> 
>    /* Make sure it gets closed again.  */
> @@ -2903,7 +2938,7 @@ proc_iterate_over_mappings (int (*func)
>         {
>           sprintf (name, "/proc/%d/object/%s", pi->pid, map->pr_mapname);
>           /* Note: caller's responsibility to close this fd!  */
> -         fd = open (name, O_RDONLY);
> +         fd = open_with_retry (name, O_RDONLY);
>           /* Note: we don't test the above call for failure;
>              we just pass the FD on as given.  Sometimes there is
>              no file, so the ioctl may return failure, but that's
> @@ -2982,7 +3017,7 @@ proc_get_LDT_entry (procinfo *pi, int ke
> 
>    /* Open the file descriptor for the LDT table.  */
>    sprintf (pathname, "/proc/%d/ldt", pi->pid);
> -  if ((fd = open (pathname, O_RDONLY)) < 0)
> +  if ((fd = open_with_retry (pathname, O_RDONLY)) < 0)
>      {
>        proc_warn (pi, "proc_get_LDT_entry (open)", __LINE__);
>        return NULL;

Re: [PATCH RFA #2] Retry open()s in procfs.c

[Kevin Buettner]

On Apr 13,  6:55pm, Kevin Buettner wrote:

> 	* procfs.c (open_with_retry): New function.
> 	(open_procinfo_files, load_syscalls, proc_iterate_over_mappings)
> 	(proc_get_LDT_entry): Call open_with_retry() instead of open().

Committed.