Re: [PATCH] stream fixes for migration

Re: [PATCH] stream fixes for migration

[Mark Williamson]

> doing anything to a c stream will work in a multithreaded environment; ie,
> the internal buffers/vars will not be stompped on.
>
> It's in the spec(don't ask me where, I read it in the info docs).

I think the issue is with Xfrd's IOStream type (libxutil/iostream.h), rather 
than with standard C streams.

(and I don't know if they're safe for multiple users, although I would expect 
that they're single thread-oriented)

Cheers,
Mark

[PATCH] stream fixes for migration

[Charles Coffing]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

I've attached a patch for libxutil/libxc.  This fixes one of the hangs I've seen during migrations.  It applies against 2.0 and 2.0-testing.

Changes:
 * Encountering EOF or error when xfrd reads from stream could cause an infinite loop.
 * Cleaned up the closing of streams.
 * Fixed several memory leaks.

Please consider applying.

Signed-off-by: Charles Coffing <ccoffing@novell.com>


[-- Attachment #2: xen-libxutil.diff --]
[-- Type: text/plain, Size: 5969 bytes --]

--- a/tools/libxutil/file_stream.c	2005-03-10 12:52:15.000000000 -0700
+++ b/tools/libxutil/file_stream.c	2005-04-01 10:15:17.279890830 -0700
@@ -151,7 +157,12 @@
  * @return result of the close
  */
 static int file_close(IOStream *s){
-    return fclose(get_file(s));
+    int result = 0;
+    if (s->data){
+        result = fclose(get_file(s));
+        s->data = (void*)0;
+    }
+    return result;
 }

 /** Free a file stream.
@@ -159,7 +170,7 @@
  * @param s file stream
  */
 static void file_free(IOStream *s){
-    // Do nothing - fclose does it all?
+    file_close(s);
 }

 /** Create an IOStream for a stream.
@@ -189,7 +200,6 @@
 	io = file_stream_new(fin);
 	if(!io){
 	    fclose(fin);
-	    //free(fin); // fclose frees ?
 	}
     }
     return io;
@@ -199,13 +209,16 @@
  *
  * @param fd file descriptor
  * @param flags giving the mode to open in (as for fdopen())
- * @return new stream for the open file, or 0 if failed
+ * @return new stream for the open file, or 0 if failed.  Always takes
+ *         ownership of fd.
  */
 IOStream *file_stream_fdopen(int fd, const char *flags){
     IOStream *io = 0;
     FILE *fin = fdopen(fd, flags);
     if(fin){
-	io = file_stream_new(fin);
+        io = file_stream_new(fin);
+        if(!io)
+            fclose(fin);
     }
     return io;
 }
--- a/tools/libxutil/gzip_stream.c	2005-03-10 12:52:13.000000000 -0700
+++ b/tools/libxutil/gzip_stream.c	2005-04-01 09:38:54.162619857 -0700
@@ -107,7 +107,12 @@
  * @return result of the close
  */
 static int gzip_close(IOStream *s){
-    return gzclose(get_gzfile(s));
+    int result = 0;
+    if (s->data){
+        result = gzclose(get_gzfile(s));
+        s->data = (void*)0;
+    }
+    return result;
 }

 /** Free a gzip stream.
@@ -115,7 +120,7 @@
  * @param s gzip stream
  */
 static void gzip_free(IOStream *s){
-    // Do nothing - fclose does it all?
+    gzip_close(s);
 }

 /** Create an IOStream for a gzip stream.
@@ -143,11 +148,10 @@
     gzFile *fgz;
     fgz = gzopen(file, flags);
     if(fgz){
-	io = gzip_stream_new(fgz);
-	if(!io){
-	    gzclose(fgz);
-	    //free(fgz); // gzclose frees ?
-	}
+        io = gzip_stream_new(fgz);
+        if(!io){
+            gzclose(fgz);
+        }
     }
     return io;
 }
@@ -156,14 +160,17 @@
  *
  * @param fd file descriptor
  * @param flags giving the mode to open in (as for fdopen())
- * @return new stream for the open file, or NULL if failed
+ * @return new stream for the open file, or NULL if failed.  Always takes
+ *         ownership of fd.
  */
 IOStream *gzip_stream_fdopen(int fd, const char *flags){
     IOStream *io = NULL;
     gzFile *fgz;
     fgz = gzdopen(fd, flags);
     if(fgz){
-	io = gzip_stream_new(fgz);
+        io = gzip_stream_new(fgz);
+        if(!io)
+            gzclose(fgz);
     }
     return io;
 }
--- a/tools/libxutil/iostream.h	2005-03-10 12:52:12.000000000 -0700
+++ b/tools/libxutil/iostream.h	2005-04-01 08:23:41.050368722 -0700
@@ -105,8 +105,11 @@
  * @return if ok, number of bytes read, otherwise negative error code
  */
 static inline int IOStream_read(IOStream *stream, void *buf, size_t n){
-    int result = 0;
-    if(stream->closed) goto exit;
+    int result;
+    if(stream->closed){
+        result = IOSTREAM_EOF;
+        goto exit;
+    }
     if(!stream->methods || !stream->methods->read){
         result = -EINVAL;
         goto exit;
@@ -124,11 +127,14 @@
  * @param stream input
  * @param buf where to put input
  * @param n number of bytes to write
- * @return if ok, number of bytes read, otherwise negative error code
+ * @return if ok, number of bytes written, otherwise negative error code
  */
 static inline int IOStream_write(IOStream *stream, const void *buf, size_t n){
-    int result = 0;
-    if(stream->closed) goto exit;
+    int result;
+    if(stream->closed){
+        result = IOSTREAM_EOF;
+        goto exit;
+    }
     if(!stream->methods || !stream->methods->write){
         result = -EINVAL;
         goto exit;
@@ -179,6 +185,7 @@
     int err = 1;
     if(stream->methods && stream->methods->close){
         err = stream->methods->close(stream);
+        stream->closed = 1;
     }
     return err;
 }
@@ -189,7 +196,7 @@
  * @return 1 if closed, 0 otherwise
  */
 static inline int IOStream_is_closed(IOStream *stream){
-  return stream->closed;
+    return stream->closed;
 }

 /** Free the memory used by the stream.
@@ -197,11 +204,14 @@
  * @param stream to free
  */
 static inline void IOStream_free(IOStream *stream){
-  if(stream->methods && stream->methods->free){
-    stream->methods->free(stream);
-  }
-  *stream = (IOStream){};
-  deallocate(stream);
+    if(!stream->closed && stream->methods && stream->methods->close){
+        stream->methods->close(stream);
+    }
+    if(stream->methods && stream->methods->free){
+        stream->methods->free(stream);
+    }
+    *stream = (IOStream){};
+    deallocate(stream);
 }


--- a/tools/libxc/xc_io.h	2005-03-10 12:52:13.000000000 -0700
+++ b/tools/libxc/xc_io.h	2005-04-01 11:18:19.119186820 -0700
@@ -45,14 +45,14 @@
     int rc;

     rc = IOStream_read(ctxt->io, buf, n);
-    return (rc == n ? 0 : rc);
+    return (rc == n ? 0 : -1);
 }

 static inline int xcio_write(XcIOContext *ctxt, void *buf, int n){
     int rc;

     rc = IOStream_write(ctxt->io, buf, n);
-    return (rc == n ? 0 : rc);
+    return (rc == n ? 0 : -1);
 }

 static inline int xcio_flush(XcIOContext *ctxt){


--- a/tools/libxc/xc_linux_save.c	2005-03-10 12:52:16.000000000 -0700
+++ b/tools/libxc/xc_linux_save.c	2005-04-01 11:42:18.509138396 -0700
@@ -172,7 +172,6 @@
     struct timeval now;
     struct timespec delay;
     long long delta;
-    int rc;

     if (START_MBIT_RATE == 0)
 	return xcio_write(ioctxt, buf, n);
@@ -212,8 +211,7 @@
 	    }
 	}
     }
-    rc = IOStream_write(ioctxt->io, buf, n);
-    return (rc == n ? 0 : rc);
+    return xcio_write(ioctxt, buf, n);
 }

 static int print_stats( int xc_handle, u32 domid,

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] stream fixes for migration

[Ian Pratt]

> I've attached a patch for libxutil/libxc.  This fixes one of 
> the hangs I've seen during migrations.  It applies against 
> 2.0 and 2.0-testing.
> 
> Changes:
>  * Encountering EOF or error when xfrd reads from stream 
> could cause an infinite loop.
>  * Cleaned up the closing of streams.
>  * Fixed several memory leaks.
> 
> Please consider applying.
> 
> Signed-off-by: Charles Coffing <ccoffing@novell.com>

Thanks!

Ian 

Re: [PATCH] stream fixes for migration

[David Hopwood]

Charles Coffing wrote:
> I've attached a patch for libxutil/libxc.  This fixes one of the hangs I've seen during migrations.  It applies against 2.0 and 2.0-testing.
> 
> Changes:
>  * Encountering EOF or error when xfrd reads from stream could cause an infinite loop.
>  * Cleaned up the closing of streams.
>  * Fixed several memory leaks.

Is it possible for IOStreams to be used concurrently, or are they only
used in a single-threaded manner?

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>

Re: [PATCH] stream fixes for migration

[Adam Heath]

On Tue, 5 Apr 2005, David Hopwood wrote:

> Charles Coffing wrote:
> > I've attached a patch for libxutil/libxc.  This fixes one of the hangs I've seen during migrations.  It applies against 2.0 and 2.0-testing.
> >
> > Changes:
> >  * Encountering EOF or error when xfrd reads from stream could cause an infinite loop.
> >  * Cleaned up the closing of streams.
> >  * Fixed several memory leaks.
>
> Is it possible for IOStreams to be used concurrently, or are they only
> used in a single-threaded manner?

doing anything to a c stream will work in a multithreaded environment; ie, the
internal buffers/vars will not be stompped on.

It's in the spec(don't ask me where, I read it in the info docs).

Re: [PATCH] stream fixes for migration

[David Hopwood]

Mark Williamson wrote:
>>doing anything to a c stream will work in a multithreaded environment; ie,
>>the internal buffers/vars will not be stompped on.
>>
>>It's in the spec(don't ask me where, I read it in the info docs).
> 
> I think the issue is with Xfrd's IOStream type (libxutil/iostream.h), rather 
> than with standard C streams.
> 
> (and I don't know if they're safe for multiple users, although I would expect 
> that they're single thread-oriented)

Well, the patch seemed to be assuming that they don't need to be safe for
multiple threads. For example, the patch tries to make multiple closes safe,
but this won't work if the closes are concurrent. I was just checking that
no-one was expecting this to work.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>