Ayal Baron has posted comments on this change.
Change subject: core: Grayed-Out LUNs Support
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/config.sql
Line 104: select fn_db_add_config_value('FilteringLUNsEnabled','true','2.2');
took me a while but I got what was bugging me here.
This should not be in the db at all.
we are solving a bug here, once we reach version 4 where we do not support 3.0
clusters then Filtering will be meaningless and instead of just deleting some
code we will have it in the db.
This should be code side logic only.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetDeviceListQuery.java
Line 30: getParameters().getVdsId(), getUserID(),
getParameters().isFiltered());
I don't understand what 'isFiltered' is doing here when getting a storage_pool
object.
First of all, this change has nothing to do with the patch subject and should
be separated into another patch.
Second, if you enforce security filtering of objects then it should not be
optional. If the filtering is done db side (as is the implementation here)
then it should do so for *all* users and admins should just have the relevant
permissions in the db.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/LUNs.java
Line 295: inUse = value;
remind me why we need this if we have storageDomainId which would be null if
not part of an SD?
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1151: FilteringLUNsEnabled(360),
Shouldn't this be annotated?
@TypeConverterAttribute(Boolean.class)
although same as db, I'm not sure why we should have a config option for this.
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/GetDeviceListVDSCommand.java
Line 33: if (!filteringLUNsEnabled) {
why if and not just:
options.add(VdsProperties.includePartitioned, filteringLUNsEnabled.toString());
--
To view, visit http://gerrit.ovirt.org/4858
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia716e72c6caa097b42d8864ab23144149d360df2
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
