Re: [Libreoffice] [PUSHED] More read(2) / write(2) cleanup in OSL/unx

Hi Michael,

>        Thanks for that - I've pushed them. They need a little more work though
> (although they are no worse than the original ;-):
>
>        a) safeWrite needs to check for EINTR as safeRead does.

Atttached is a patch should solve this.

>        b) neither safeRead or safeWrite advance the buffer /
>           data pointer when they get a partial read/write - which
>           is broken.
>                + we should have an unsigned char *ptr for both of
>                  those, and add nWritten or nRead to it each time
>                  we get a partial chunk read.

Ouch, good catch! Attached is another patch for this.

The changes are again available under LGPLv3+/MPL.

>        Otherwise this is nice :-) It would be sexy to have someone (when this
> is all fixed), to isolate and unify all the temp file creation code that
> is used all over the place; not all of it is best practise, and there is
> a lot of dangerous duplication.

Sounds like a worthy goal, do you have some pointers to areas that
duplicate the code?

Thanks,
Julien

From 219182324770e28d12b7ee179af1c5c594ea6da1 Mon Sep 17 00:00:00 2001
From: Julien Chaffraix <julien.chaffraix@gmail.com>
Date: Sat, 30 Apr 2011 16:13:22 -0700
Subject: [PATCH 1/2] Handled EINTR in safeWrite.

This makes us match safeRead's behavior. Spotted by Michael Meeks.
---
 sal/osl/unx/readwrite_helper.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/sal/osl/unx/readwrite_helper.c b/sal/osl/unx/readwrite_helper.c
index 41aa41e..51e1ec6 100644
--- a/sal/osl/unx/readwrite_helper.c
+++ b/sal/osl/unx/readwrite_helper.c
@@ -39,9 +39,14 @@ sal_Bool safeWrite(int fd, void* data, sal_uInt32 dataSize)
     OSL_ASSERT(dataSize == (sal_uInt32)nToWrite);
     while ( nToWrite ) {
         sal_Int32 nWritten = write(fd, data, nToWrite);
-        if ( nWritten < 0 )
+        if ( nWritten < 0 ) {
+            if ( errno == EINTR )
+                continue;
+
             return sal_False;
 
+        }
+
         OSL_ASSERT(nWritten > 0);
         nToWrite -= nWritten;
     }
-- 
1.7.1

From 649c8feca144ff212e3e182ac9c0bb7a66f28ed9 Mon Sep 17 00:00:00 2001
From: Julien Chaffraix <julien.chaffraix@gmail.com>
Date: Sat, 30 Apr 2011 16:37:49 -0700
Subject: [PATCH 2/2] Properly advance the data / buffer pointer.

Spotted by Michael Meeks.
---
 sal/osl/unx/readwrite_helper.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/sal/osl/unx/readwrite_helper.c b/sal/osl/unx/readwrite_helper.c
index 51e1ec6..e259895 100644
--- a/sal/osl/unx/readwrite_helper.c
+++ b/sal/osl/unx/readwrite_helper.c
@@ -35,10 +35,12 @@
 sal_Bool safeWrite(int fd, void* data, sal_uInt32 dataSize)
 {
     sal_Int32 nToWrite = dataSize;
+    unsigned char* dataToWrite = data;
+
     // Check for overflow as we convert a signed to an unsigned.
     OSL_ASSERT(dataSize == (sal_uInt32)nToWrite);
     while ( nToWrite ) {
-        sal_Int32 nWritten = write(fd, data, nToWrite);
+        sal_Int32 nWritten = write(fd, dataToWrite, nToWrite);
         if ( nWritten < 0 ) {
             if ( errno == EINTR )
                 continue;
@@ -49,6 +51,7 @@ sal_Bool safeWrite(int fd, void* data, sal_uInt32 dataSize)
 
         OSL_ASSERT(nWritten > 0);
         nToWrite -= nWritten;
+        dataToWrite += nWritten;
     }
 
     return sal_True;
@@ -57,10 +60,12 @@ sal_Bool safeWrite(int fd, void* data, sal_uInt32 dataSize)
 sal_Bool safeRead( int fd, void* buffer, sal_uInt32 count )
 {
     sal_Int32 nToRead = count;
+    unsigned char* bufferForReading = buffer;
+
     // Check for overflow as we convert a signed to an unsigned.
     OSL_ASSERT(count == (sal_uInt32)nToRead);
     while ( nToRead ) {
-        sal_Int32 nRead = read(fd, buffer, nToRead);
+        sal_Int32 nRead = read(fd, bufferForReading, nToRead);
         if ( nRead < 0 ) {
             // We were interrupted before reading, retry.
             if (errno == EINTR)
@@ -75,6 +80,7 @@ sal_Bool safeRead( int fd, void* buffer, sal_uInt32 count )
             return sal_False;
 
         nToRead -= nRead;
+        bufferForReading += nRead;
     }
 
     return sal_True;
-- 
1.7.1