File::Read does not handle short reads

[EugeneBi]

Summary

When loading huge file from network share, the pread may succeed but read smaller amount bytes than it was requested. If this happens we need to issue another pread until our request is satisfied or pread returns 0 indicating end of file.

[zturner]

source/Host/common/File.cpp

774

I know you are just fixing an existing bug, but since you're in here, the existing code really could use some love. There are two Read() methods, one which takes an offset and one which doesn't. They both are written completely differently though, even though the former might as well just call the other one with an offset of 0. Furthermore, there is an entirely separate code path when MAX_READ_SIZE is defined, even though it should be exactly the same algorithm, but reading fewer bytes on each iteration.

I won't force you to do it, but if you have some spare cycles it would be really great if this could get cleaned up. Ideally the code would look like this:

1. Read (void *buf, size_t &num_bytes) would be a 1 line function. return Read(buf, num_bytes, 0);
2. At the point where MAX_READ_SIZE is defined for apple platforms, have an #else branch which defines it for other platforms equal to SIZE_T_MAX.
3. Delete the #if defined(MAX_READ_SIZE) branch entirely.
4. Have the main branch only read std::min(MAX_READ_SIZE, bytes_left) bytes at a time until it's done.

This still leaves the question of what to do with the windows branch since it doesn't have pread. If you're familiar with Windows you can re-write this branch yourself, or I'm willing to do it for you otherwise.

What do you think?

[clayborg]

We can probably actually get rid of the function that takes the offset.

Error File::Read (void *buf, size_t &num_bytes, off_t &offset);

The only clients are internal from within File.cpp, or clients where they pass in zero as the offset so they can actually use the normal read function:

Error File::Read (void *buf, size_t &num_bytes);

Then we just need to make sure the above function works correctly.

[clayborg]

Then it will keep Windows happy using just the normal "read" instead of "pread".

[EugeneBi]

I'll take a look

[EugeneBi]

I took a look.

The no-offset Read that Greg mentions suffers from the same bug, i.e. it better be fixed too. Now, what Greg says - prove that we always call with offset zero and eliminate the other two methods - makes total sense to me. Well, since I did not run into the first Read, it means that there are callers that do not use it but use the combination of the other two. So, doing the merge that Greg suggests involves fixing all current callers, and this is a bit deeper than I am comfortable to undertake.

Sorry, I do not feel that I can do it.

Eugene

source/Host/common/File.cpp

if (num_bytes > MAX_READ_SIZE)
         return error;
     }
 #endif
 
 #ifndef _WIN32
     int fd = GetDescriptor();
     if (fd != kInvalidDescriptor)
     {
+        size_t bytes_left = num_bytes;
+        num_bytes = 0;
+        char *pos = (char*)buf;
+        while (bytes_left > 0)
+        {
             ssize_t bytes_read = -1;
             do
             {
-            bytes_read = ::pread (fd, buf, num_bytes, offset);
+                bytes_read = ::pread (fd, pos, bytes_left, offset);
             } while (bytes_read < 0 && errno == EINTR);
 
             if (bytes_read < 0)
             {
                 num_bytes = 0;
                 error.SetErrorToErrno();
+                break;
+            }
+            else if (bytes_read == 0)
+            {
+                break;
             }
             else
             {
                 offset += bytes_read;
-            num_bytes = bytes_read;
+                num_bytes += bytes_read;
+                bytes_left -= bytes_read;
+                pos += bytes_read;
+            }
         }
     }
     else 
     {
         num_bytes = 0;
         error.SetErrorString("invalid file handle");
     }
 #else