Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: <derby-dev@db.apache.org>
Message-ID: <15349537.1174834593355.JavaMail.jira@brutus>
Date: Sun, 25 Mar 2007 07:56:33 -0700 (PDT)
From: "Knut Anders Hatlen (JIRA)" <jira@apache.org>
To: derby-dev@db.apache.org
Subject: [jira] Commented: (DERBY-2379) provide encryption support for
 temporary files used by lob if the dara base is encrypted
In-Reply-To: <20644399.1172605566227.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/DERBY-2379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483945 ] 

Knut Anders Hatlen commented on DERBY-2379:
-------------------------------------------

* It seems to me LOBFile implements all the methods of the
  StorageRandomAccessFile interface, but the class doesn't implement
  the interface. Would it make sense to let LOBFile implement
  StorageRandomAccessFile? That would require fewer changes in
  LOBStreamControl, and one could also leave the non-encrypted
  read/write methods out of LOBFile.

* It would be good to add some comments explaining the purpose of the
  instance variables and the private methods in LOBFile.

* Is the lobFile field in LOBFile necessary? It doesn't seem to be
  used outside the constructor.

* LOBFile.getBlocks:
  Is it necessary to check whether len is negative and thrown an
  IndexOutOfBoundsException? I guess the boundaries are checked on a
  higher level as well, and you'll get a NegativeArraySizeException
  anyway if len in negative, so I don't see any need to explicitly
  throw IndexOutOfBoundsException.

* LOBFile.getBlocks:
  Couldn't the calculation of endPos be expressed without the
  condition, like this:
    long endPos = (pos + len + blockSize - 1) / blockSize * blockSize;
  I'm not saying it will improve the readability significantly,
  though... :)

* LOBFile.writeEncrypted(byte):
    tailSize = (pos > tailSize - 1) ? pos + 1 : tailSize;
  Perhaps this is clearer when written like this:
    if (pos >= tailSize) tailSize = pos + 1;

* LOBFile.writeEncrypted(byte):
+                long l = randomAccessFile.length();
+                tailSize = 0;
+            }
  The variable l is never used.

* The read*/write* methods of LOBFile have many calls to
  RandomAccessFile.length(). Would it be possible to reduce the number
  of times it is called? I'm not sure, but I suspect length() might
  involve a system call, so one shouldn't call it too frequently. I
  was thinking perhaps we could change code that looks like this
+        if (currentPos >= randomAccessFile.length()) {
+            //current postion is in memory
+            int pos = (int) (currentPos - randomAccessFile.length());
into something like this:
    long fileLength = randomAccessFile.length();
    if (currentPos >= fileLength) {
        int pos = (int) (currentPos - fileLength);

* LOBFile.writeEncrypted(byte[],int,int):
+        if (finalPos < blockSize) {
+            //updated size won't be enough to perform encryption'
+            System.arraycopy (b, off, tail, pos, len);
+            tailSize = pos + len;
+            currentPos += len;
+            return;
+        }
Shouldn't tailSize be max(tailSize, pos+len)?

and

+        //copy the bytes from tail which won't be overwritten'
+        System.arraycopy (tail, 0, clearText, 0, pos);
Shouldn't the last argument have been tailLength in case we are not
overwriting the end of the tail?

* LOBFile.readEncrypted():
  It doesn't seem like currentPos is incremented in the case where we
  need to read from the file.

* LOBFile.readEncrypted(byte[],int,int) / writeEncrypted(byte[],int,int):
  It's probably slightly faster to replace this kind of loops
+            for (int i = 0; i < cypherText.length / blockSize; i++) {
+                df.decrypt (cypherText, i * blockSize, blockSize, tmpByte, 
+                                                                i * blockSize);
+            }
  with
    for (int offset = 0; offset < cypherText.length; offset += blockSize) {
        df.decrypt(cypherText, offset, blockSize, tmpByte, offset);
    }

* LOBFile.readEncrypted(byte[],int,int):
  currentPos is not incremented when overFlow == 0.

* LOBFile.readEncrypted(byte[],int,int):
  This part doesn't look correct to me:
+            //find out total number of bytes we can read
+            int newLen = (len - cypherText.length < tailSize) 
+                            ? len - cypherText.length
+                            : tailSize;
  I think it only works if the read starts on a block boundary, and it
  should have used overflow instead of (len - cypherText.length).

* I don't understand why getFilePointer, seek, read(byte[],off,len)
  and setLength are synchronized. Could you explain why these methods
  need synchronization, and why the other methods don't?

* LOBFile.setLength() is a no-op if the new length is greater than the
  old length. Wouldn't it be better to throw an error in this case?
  It's not supposed to happen since it's only called from
  LOBStreamControl.truncate(), but since setLength() is already
  checking that the new size is not greater, I think it's better to
  throw an IllegalArgumentException or UnsupportedOperationException
  than to silently return.

* I got these warnings when I tried to generate the javadoc:
  [javadoc] .../LOBFile.java:254: warning - @return tag has no arguments.
  [javadoc] .../LOBFile.java:346: warning - @return tag has no arguments.
  [javadoc] .../DataFactory.java:381: warning - @return tag has no arguments.

* If my comments have revealed bugs in the patch, I think it would be
  good to write test cases which exposes them and add them to the
  JUnit tests.

> provide encryption support for temporary files used by lob if the dara base is encrypted
> ----------------------------------------------------------------------------------------
>
>                 Key: DERBY-2379
>                 URL: https://issues.apache.org/jira/browse/DERBY-2379
>             Project: Derby
>          Issue Type: Sub-task
>    Affects Versions: 10.3.0.0
>         Environment: all
>            Reporter: Anurag Shekhar
>         Assigned To: Anurag Shekhar
>         Attachments: derby-2379.diff
>
>


-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.