Liron Aravot has posted comments on this change. Change subject: core: WAD should be Ignored on File Domain Disks ......................................................................
Patch Set 3: Code-Review-1 (9 comments) http://gerrit.ovirt.org/#/c/31687/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/SecureDeletionHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/SecureDeletionHandler.java: Line 4: import org.ovirt.engine.core.common.vdscommands.SecureDeletion; Line 5: import org.ovirt.engine.core.common.vdscommands.StorageDomainIdParametersBase; Line 6: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 7: Line 8: public class SecureDeletionHandler { please rename the class, I don't think that "SecureDeletion" correct name. I'd suggest WipeAfterDeleteHandler. Line 9: Line 10: /** Line 11: * Since the file system is responsible for handling block allocation, there is no need Line 12: * for posting zeros on file domains. This method gets the parameters of a command that may Line 5: import org.ovirt.engine.core.common.vdscommands.StorageDomainIdParametersBase; Line 6: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 7: Line 8: public class SecureDeletionHandler { Line 9: please add private ctor to this class, there's no need to allow to create instances of it. Line 10: /** Line 11: * Since the file system is responsible for handling block allocation, there is no need Line 12: * for posting zeros on file domains. This method gets the parameters of a command that may Line 13: * post zeros on the storage and fixes its postZero value if required. Line 14: * @param parameters The parameters of the command that should be executed. Line 15: * @param <T> The parameters type. Line 16: * @return The fixed parameters. Line 17: */ Line 18: public static <T extends StorageDomainIdParametersBase & SecureDeletion> any reason to not have here just SecureDeletion? Line 19: T fixParametersWithSecureDeletion(T parameters) { Line 20: StorageDomainStatic storageDomainStatic = Line 21: DbFacade.getInstance().getStorageDomainStaticDao().get(parameters.getStorageDomainId()); Line 22: return fixParametersWithSecureDeletion(parameters, storageDomainStatic.getStorageType().isFileDomain()); Line 15: * @param <T> The parameters type. Line 16: * @return The fixed parameters. Line 17: */ Line 18: public static <T extends StorageDomainIdParametersBase & SecureDeletion> Line 19: T fixParametersWithSecureDeletion(T parameters) { please rename as well Line 20: StorageDomainStatic storageDomainStatic = Line 21: DbFacade.getInstance().getStorageDomainStaticDao().get(parameters.getStorageDomainId()); Line 22: return fixParametersWithSecureDeletion(parameters, storageDomainStatic.getStorageType().isFileDomain()); Line 23: } Line 21: DbFacade.getInstance().getStorageDomainStaticDao().get(parameters.getStorageDomainId()); Line 22: return fixParametersWithSecureDeletion(parameters, storageDomainStatic.getStorageType().isFileDomain()); Line 23: } Line 24: Line 25: protected static <T extends StorageDomainIdParametersBase & SecureDeletion> same Line 26: T fixParametersWithSecureDeletion(T parameters, boolean isFileDomain) { Line 27: if (isFileDomain) { Line 28: parameters.setPostZero(false); Line 29: } Line 22: return fixParametersWithSecureDeletion(parameters, storageDomainStatic.getStorageType().isFileDomain()); Line 23: } Line 24: Line 25: protected static <T extends StorageDomainIdParametersBase & SecureDeletion> Line 26: T fixParametersWithSecureDeletion(T parameters, boolean isFileDomain) { suggestion: this method is better to get as the second parameter StorageDomainStatic, that way it'll be easier to call it from the outside. Line 27: if (isFileDomain) { Line 28: parameters.setPostZero(false); Line 29: } Line 30: return parameters; http://gerrit.ovirt.org/#/c/31687/3/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/SecureDeletionHandlerTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/SecureDeletionHandlerTest.java: Line 41: parameters.setPostZero(false); Line 42: assertPostZeroValue(false, false); Line 43: } Line 44: Line 45: private void assertPostZeroValue(boolean isFileDomain, boolean postZeroExpectedValue) { please change the name to reflect that this method don't just asserts. Line 46: SecureDeletionHandler.fixParametersWithSecureDeletion(parameters, isFileDomain); Line 47: assertEquals(MessageFormat.format( Line 48: "Wrong VDS command parameters: 'postZero' should be {0}.", postZeroExpectedValue), Line 49: parameters.getPostZero(), postZeroExpectedValue); Line 48: "Wrong VDS command parameters: 'postZero' should be {0}.", postZeroExpectedValue), Line 49: parameters.getPostZero(), postZeroExpectedValue); Line 50: } Line 51: Line 52: private class ParametersWithSecureDeletion extends StorageDomainIdParametersBase implements SecureDeletion { should be static class Line 53: Line 54: private boolean postZero; Line 55: Line 56: @Override http://gerrit.ovirt.org/#/c/31687/3/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/SecureDeletion.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/SecureDeletion.java: Line 1: package org.ovirt.engine.core.common.vdscommands; Line 2: Line 3: /** Line 4: * In a VDS command parameters object, this interface Line 5: * represents the ability to delete storage securely. please change it to WAD as well, it's not secured. Line 6: */ Line 7: public interface SecureDeletion { Line 8: Line 9: boolean getPostZero(); -- To view, visit http://gerrit.ovirt.org/31687 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0df2c07282994557e149db922cc1d3a166c5aa8f Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Idan Shaby <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
