Juan Hernandez has posted comments on this change.
Change subject: core: Add BLOB servlet
......................................................................
Patch Set 3: (2 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/blob/BlobRegistry.java
Line 52: BlobServer server = serverReference.get();
The reason to use the atomic reference here is to allow sharing it between two
threads. Without the AtomicReference the following extremely unlikely situation
could happen:
1. The thread that initializes the BLOB servlet updates the variable
registering itself doing "server = this". But the Java memory model doesn't
demand that this update is immediately visible to other threads, the Java
virtual machine and JIT could decide to keep it an registry in the CPU, for
example. Eventually the value will be written to main memory. A typical
solution for this is to use the "volatile" modifier, but that is not 100% safe
either. The safe bet is to use synchronization: the Java memory model does
demand that these updates are made visible to other threads (written to main
memory) when a synchronization primitive is used.
2. The thread that registers the URL finds that the reference is still null,
when it has in fact already been changed by the servlet, but not written to
main memory yet.
As I said this is extremely unlikely to happen, because the servlet is loaded
when the application is deployed and the call to the BlobRegistry is an user
initiated action, that can happen after the application is deployed. Both
threads will probably (with very high probability) cross a synchronization
point before hitting this code.
In a previous patch set I used a list and then shared it using the synchronized
modifier in the methods, so when I replaced it with a simple reference it
seemed natural to use AtomicReference instead. This is probably
overcomplicated, I am pretty sure that using a plain reference will work as
well.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/blob/BlobServlet.java
Line 102: public URL registerBlob (File blob) {
Yes, the file comes from a trusted source inside the engine, from the code that
updates the installation of the node. The file will always be something like
"/usr/share/ovirt-node-iso/something.iso".
However sanitizing the path doesn't hurt. Do you think that we should use
something similar to what we did in the FileServlet?
--
To view, visit http://gerrit.ovirt.org/6484
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4726aa4084ebb8f93caf0616aceab10957c16b90
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Grant Murphy <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
