harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Deakin <oliver.dea...@googlemail.com>
Subject Re: [classlib][luni] calling mmap
Date Tue, 29 Sep 2009 14:40:33 GMT
Tim Ellison wrote:
> Sorry for talking so long to raise this...
>
> On 16/Sep/2009 23:44, odeakin@apache.org wrote:
>   
>> Author: odeakin
>> Date: Wed Sep 16 22:44:49 2009
>> New Revision: 815994
>>
>> URL: http://svn.apache.org/viewvc?rev=815994&view=rev
>> Log:
>> Apply patch for HARMONY-6315 ([classlib][nio] FileChannel.map throws IOException
when called with size 0)
>>
>> Modified:
>>     harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java
>>     harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c
>>     harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
>>
>> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java?rev=815994&r1=815993&r2=815994&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java
(original)
>> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/org/apache/harmony/luni/platform/OSMemory.java
Wed Sep 16 22:44:49 2009
>> @@ -551,6 +551,11 @@
>>  
>>  	public long mmap(long fileDescriptor, long alignment, long size,
>>  			int mapMode) throws IOException {
>> +		// if size is 0, call to mmap will fail, so return dummy address
>> +		if (size == 0)
>> +		{
>> +			return 0;
>> +		}
>>  		long address = mmapImpl(fileDescriptor, alignment, size, mapMode);
>>  		if (address == -1) {
>>  			throw new IOException();
>>     
>
> Hmm, I'm not convinced this is the right solution.
>
> Making size == 0 a special case that always succeeds (even if it is
> returning a bogus address) bypasses all the other checks that the mmap
> function call would have made.
>
> For example, what if the file descriptor was invalid? or points to an
> unmappable file?  Then I would expect an IOException.
>   

I think (from the JIRA) that we were getting EINVAL back from the mmap 
call with size 0, and that's why it was fixed before making the mmap 
call. It would be good to know what the RI does in the case where size 
is 0 and we're also mapping an invalid file - do we get an exception 
thrown? Perhaps Catherine can clarify?

> There is nothing in the man pages to say that size = 0 is invalid.
>   

No, but in the JIRA it indicates that we receive an EINVAL return code 
when passing in size 0, so perhaps it is just not in the man page. The 
man page I am looking at also didn't explicitly say negative values are 
illegal, but I think we can assume that's true.

> Plus, there is now no path that will return -1 as the address, so you
> can remove the check (all the exceptions are raised in the native code).
>   

That's true, I hadn't spotted that. I also noticed a bug in the native 
code change where we allocate 100 chars for the error string but the 
EAGAIN error message is 101 characters long (plus another 1 for the null 
terminator) so we're off by 2 there. Perhaps a neater way to get these 
error messages would be to use strerror() once we have the error number, 
but for now Ill fix this array size bug and remove the redundant -1 
check in the java code for now, and we can keep discussing the correct 
behaviour here.

Regards,
Oliver

> Regards,
> Tim
>
>   
>> Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c?rev=815994&r1=815993&r2=815994&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c
(original)
>> +++ harmony/enhanced/classlib/trunk/modules/luni/src/main/native/luni/unix/OSMemoryLinux32.c
Wed Sep 16 22:44:49 2009
>> @@ -26,9 +26,11 @@
>>  #include <sys/mman.h>
>>  #include <errno.h>
>>  #include <unistd.h>
>> +#include <string.h>
>>  #include "vmi.h"
>>  #include "OSMemory.h"
>>  #include "IMemorySystem.h"
>> +#include "exceptions.h"
>>  
>>  #ifdef ZOS
>>  #define FD_BIAS 1000
>> @@ -157,6 +159,7 @@
>>    //PORT_ACCESS_FROM_ENV (env);
>>    void *mapAddress = NULL;
>>    int prot, flags;
>> +  char errorString[100];
>>  
>>    // Convert from Java mapping mode to port library mapping mode.
>>    switch (mmode)
>> @@ -174,12 +177,41 @@
>>  	flags = MAP_PRIVATE;
>>          break;
>>        default:
>> +    sprintf(errorString, "Map mode %d not recognised", mmode);
>> +    throwJavaIoIOException(env, errorString);
>>          return -1;
>>      }
>>  
>>    mapAddress = mmap(0, (size_t)(size&0x7fffffff), prot, flags, fd-FD_BIAS, (off_t)(alignment&0x7fffffff));
>>    if (mapAddress == MAP_FAILED)
>>      {
>> +      switch (errno)
>> +        {
>> +        case EACCES:
>> +          strcpy(errorString, "Call to mmap failed - a file descriptor refers to
a non-regular file.");
>> +          break;
>> +        case EAGAIN:
>> +          strcpy(errorString, "Call to mmap failed with error EAGAIN - the file
has been locked, or too much memory has been locked.");
>> +          break;
>> +        case EBADF:
>> +          strcpy(errorString, "Call to mmap failed with error EBADF - invalid file
descriptor");
>> +          break;
>> +        case EINVAL:
>> +          strcpy(errorString, "Call to mmap failed with error EINVAL - invalid start,
length or offset.");
>> +          break;
>> +        case ENFILE:
>> +          strcpy(errorString, "Call to mmap failed with error ENFILE - number of
open files has reached the system limit.");
>> +          break;
>> +        case ENODEV:
>> +          strcpy(errorString, "Call to mmap failed with error ENODEV - filesystem
does not support memory mapping.");
>> +          break;
>> +        case ENOMEM:
>> +          strcpy(errorString, "Call to mmap failed with error ENOMEM - no memory
is available");
>> +          break;
>> +        default:
>> +          sprintf(errorString, "Call to mmap returned with errno %d", errno);
>> +        }
>> +      throwJavaIoIOException(env, errorString);
>>        return -1;
>>      }
>>    return (jlong) ((IDATA)mapAddress);
>>
>> Modified: harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
>> URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java?rev=815994&r1=815993&r2=815994&view=diff
>> ==============================================================================
>> --- harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
(original)
>> +++ harmony/enhanced/classlib/trunk/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
Wed Sep 16 22:44:49 2009
>> @@ -22,17 +22,19 @@
>>  import java.io.FileOutputStream;
>>  import java.io.IOException;
>>  import java.io.RandomAccessFile;
>> +import java.nio.BufferUnderflowException;
>>  import java.nio.ByteBuffer;
>>  import java.nio.IntBuffer;
>>  import java.nio.MappedByteBuffer;
>>  import java.nio.channels.FileChannel;
>> +import java.nio.channels.NonWritableChannelException;
>>  import java.nio.channels.FileChannel.MapMode;
>>  
>>  import junit.framework.TestCase;
>>  
>>  public class MappedByteBufferTest extends TestCase {
>>  
>> -    File tmpFile;
>> +    File tmpFile, emptyFile;
>>      
>>      /**
>>       * A regression test for failing to correctly set capacity of underlying
>> @@ -63,6 +65,62 @@
>>      }
>>      
>>      /**
>> +     * Regression for HARMONY-6315 - FileChannel.map throws IOException
>> +     * when called with size 0
>> +     * 
>> +     * @throws IOException
>> +     */
>> +    public void testEmptyBuffer() throws IOException {
>> +    	// Map empty file
>> +        FileInputStream fis = new FileInputStream(emptyFile);
>> +        FileChannel fc = fis.getChannel();
>> +        MappedByteBuffer mmb = fc.map(FileChannel.MapMode.READ_ONLY, 0, fc.size());
>> +        
>> +        // check non-null
>> +        assertNotNull("MappedByteBuffer created from empty file should not be null",

>> +        		mmb);
>> +        
>> +        // check capacity is 0
>> +        int len = mmb.capacity();
>> +        assertEquals("MappedByteBuffer created from empty file should have 0 capacity",

>> +        		0, len);
>> +        
>> +        assertFalse("MappedByteBuffer from empty file shouldn't be backed by an
array ", 
>> +        		mmb.hasArray());
>> +        
>> +        try
>> +        {
>> +        	byte b = mmb.get();
>> +        	fail("Calling MappedByteBuffer.get() on empty buffer should throw a BufferUnderflowException");
>> +        }
>> +        catch (BufferUnderflowException e)
>> +        {
>> +        	// expected behaviour
>> +        }
>> +        
>> +        // test expected exceptions thrown
>> +        try 
>> +        {
>> +        	mmb = fc.map(FileChannel.MapMode.READ_WRITE, 0, fc.size());
>> +        	fail("Expected NonWritableChannelException to be thrown");
>> +        }
>> +        catch (NonWritableChannelException e)
>> +        {
>> +        	// expected behaviour
>> +        }
>> +        try
>> +        {
>> +        	mmb = fc.map(FileChannel.MapMode.PRIVATE, 0, fc.size());
>> +        	fail("Expected NonWritableChannelException to be thrown");
>> +        }
>> +        catch (NonWritableChannelException e)
>> +        {
>> +        	// expected behaviour
>> +        }
>> +        fc.close();
>> +    }
>> +    
>> +    /**
>>       * @tests {@link java.nio.MappedByteBuffer#force()}
>>       */
>>      public void test_force() throws IOException {
>> @@ -146,5 +204,8 @@
>>          fileChannel.write(byteBuffer);
>>          fileChannel.close();
>>          fileOutputStream.close();
>> +        
>> +        emptyFile = File.createTempFile("harmony", "test");  //$NON-NLS-1$//$NON-NLS-2$
>> +        emptyFile.deleteOnExit();
>>      }
>>  }
>>
>>
>>
>>     
>
>   

-- 
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU


Mime
View raw message