[
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.
