diff mbox

Always do locking when iterating over list of streams (bug 15142)

Message ID mvmr2w5ngox.fsf_-_@suse.de
State New, archived
Headers

Commit Message

Andreas Schwab Aug. 21, 2017, 2:22 p.m. UTC 
  [BZ #15142]
	* libio/genops.c (_IO_list_all_stamp): Delete.  All uses removed.
	(_IO_flush_all_lockp): Always lock list_all_lock.
	(_IO_flush_all_linebuffered): Likewise.
	(_IO_unbuffer_all): Likewise.

Comments

Florian Weimer Aug. 21, 2017, 2:26 p.m. UTC | #1 
On 08/21/2017 04:22 PM, Andreas Schwab wrote:
> 	[BZ #15142]
> 	* libio/genops.c (_IO_list_all_stamp): Delete.  All uses removed.
> 	(_IO_flush_all_lockp): Always lock list_all_lock.
> 	(_IO_flush_all_linebuffered): Likewise.
> 	(_IO_unbuffer_all): Likewise.

This change seems reasonable, but I think we need to remove flushing on
abort first (completely, not the hybrid attempt I posted).

Thanks,
Florian
Andreas Schwab Oct. 5, 2017, 2:51 p.m. UTC | #2 
On Aug 21 2017, Florian Weimer <fweimer@redhat.com> wrote:

> On 08/21/2017 04:22 PM, Andreas Schwab wrote:
>> 	[BZ #15142]
>> 	* libio/genops.c (_IO_list_all_stamp): Delete.  All uses removed.
>> 	(_IO_flush_all_lockp): Always lock list_all_lock.
>> 	(_IO_flush_all_linebuffered): Likewise.
>> 	(_IO_unbuffer_all): Likewise.
>
> This change seems reasonable, but I think we need to remove flushing on
> abort first (completely, not the hybrid attempt I posted).

Now that flushing is removed from abort, ok?

Andreas.
Florian Weimer Oct. 5, 2017, 3:07 p.m. UTC | #3 
On 10/05/2017 04:51 PM, Andreas Schwab wrote:
> On Aug 21 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> On 08/21/2017 04:22 PM, Andreas Schwab wrote:
>>> 	[BZ #15142]
>>> 	* libio/genops.c (_IO_list_all_stamp): Delete.  All uses removed.
>>> 	(_IO_flush_all_lockp): Always lock list_all_lock.
>>> 	(_IO_flush_all_linebuffered): Likewise.
>>> 	(_IO_unbuffer_all): Likewise.
>>
>> This change seems reasonable, but I think we need to remove flushing on
>> abort first (completely, not the hybrid attempt I posted).
> 
> Now that flushing is removed from abort, ok?

Yes, still looks good to me.

Thanks,
Florian
diff mbox

Patch

diff --git a/libio/genops.c b/libio/genops.c
index 6ad7346cae..89376d1b9b 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -38,10 +38,6 @@ 
 static _IO_lock_t list_all_lock = _IO_lock_initializer;
 #endif
 
-/* Used to signal modifications to the list of FILE decriptors.  */
-static int _IO_list_all_stamp;
-
-
 static _IO_FILE *run_fp;
 
 #ifdef _IO_MTSAFE_IO
@@ -69,16 +65,12 @@  _IO_un_link (struct _IO_FILE_plus *fp)
       if (_IO_list_all == NULL)
 	;
       else if (fp == _IO_list_all)
-	{
-	  _IO_list_all = (struct _IO_FILE_plus *) _IO_list_all->file._chain;
-	  ++_IO_list_all_stamp;
-	}
+	_IO_list_all = (struct _IO_FILE_plus *) _IO_list_all->file._chain;
       else
 	for (f = &_IO_list_all->file._chain; *f; f = &(*f)->_chain)
 	  if (*f == (_IO_FILE *) fp)
 	    {
 	      *f = fp->file._chain;
-	      ++_IO_list_all_stamp;
 	      break;
 	    }
       fp->file._flags &= ~_IO_LINKED;
@@ -106,7 +98,6 @@  _IO_link_in (struct _IO_FILE_plus *fp)
 #endif
       fp->file._chain = (_IO_FILE *) _IO_list_all;
       _IO_list_all = fp;
-      ++_IO_list_all_stamp;
 #ifdef _IO_MTSAFE_IO
       _IO_funlockfile ((_IO_FILE *) fp);
       run_fp = NULL;
@@ -794,17 +785,13 @@  _IO_flush_all_lockp (int do_lock)
 {
   int result = 0;
   struct _IO_FILE *fp;
-  int last_stamp;
 
 #ifdef _IO_MTSAFE_IO
-  __libc_cleanup_region_start (do_lock, flush_cleanup, NULL);
-  if (do_lock)
-    _IO_lock_lock (list_all_lock);
+  _IO_cleanup_region_start_noarg (flush_cleanup);
+  _IO_lock_lock (list_all_lock);
 #endif
 
-  last_stamp = _IO_list_all_stamp;
-  fp = (_IO_FILE *) _IO_list_all;
-  while (fp != NULL)
+  for (fp = (_IO_FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
       if (do_lock)
@@ -823,21 +810,11 @@  _IO_flush_all_lockp (int do_lock)
       if (do_lock)
 	_IO_funlockfile (fp);
       run_fp = NULL;
-
-      if (last_stamp != _IO_list_all_stamp)
-	{
-	  /* Something was added to the list.  Start all over again.  */
-	  fp = (_IO_FILE *) _IO_list_all;
-	  last_stamp = _IO_list_all_stamp;
-	}
-      else
-	fp = fp->_chain;
     }
 
 #ifdef _IO_MTSAFE_IO
-  if (do_lock)
-    _IO_lock_unlock (list_all_lock);
-  __libc_cleanup_region_end (0);
+  _IO_lock_unlock (list_all_lock);
+  _IO_cleanup_region_end (0);
 #endif
 
   return result;
@@ -856,16 +833,13 @@  void
 _IO_flush_all_linebuffered (void)
 {
   struct _IO_FILE *fp;
-  int last_stamp;
 
 #ifdef _IO_MTSAFE_IO
   _IO_cleanup_region_start_noarg (flush_cleanup);
   _IO_lock_lock (list_all_lock);
 #endif
 
-  last_stamp = _IO_list_all_stamp;
-  fp = (_IO_FILE *) _IO_list_all;
-  while (fp != NULL)
+  for (fp = (_IO_FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
       _IO_flockfile (fp);
@@ -875,15 +849,6 @@  _IO_flush_all_linebuffered (void)
 
       _IO_funlockfile (fp);
       run_fp = NULL;
-
-      if (last_stamp != _IO_list_all_stamp)
-	{
-	  /* Something was added to the list.  Start all over again.  */
-	  fp = (_IO_FILE *) _IO_list_all;
-	  last_stamp = _IO_list_all_stamp;
-	}
-      else
-	fp = fp->_chain;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -919,6 +884,12 @@  static void
 _IO_unbuffer_all (void)
 {
   struct _IO_FILE *fp;
+
+#ifdef _IO_MTSAFE_IO
+  _IO_cleanup_region_start_noarg (flush_cleanup);
+  _IO_lock_lock (list_all_lock);
+#endif
+
   for (fp = (_IO_FILE *) _IO_list_all; fp; fp = fp->_chain)
     {
       if (! (fp->_flags & _IO_UNBUFFERED)
@@ -961,6 +932,11 @@  _IO_unbuffer_all (void)
 	 used.  */
       fp->_mode = -1;
     }
+
+#ifdef _IO_MTSAFE_IO
+  _IO_lock_unlock (list_all_lock);
+  _IO_cleanup_region_end (0);
+#endif
 }