Idan Shaby has posted comments on this change. Change subject: core: WAD should be Ignored on File Domain Disks ......................................................................
Patch Set 3: (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. We are trying to avoid this term because we are enabling it for file domains in the UI, too (http://gerrit.ovirt.org/#/c/31212/). The concept is that if you enable the "Secure Deletion" option in the UI (which will replace the old "wipe after delete" checkbox on my next patch), then you know that this disk will be treated securly when it's deleted - file or block. This name was accepted by Sean and Allon, so it will remain the same. 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 i Done 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? Yes, this way we can use: parameters.getStorageDomainId() 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 Look above. 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 Look above. 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 StorageDom Tried that - it makes the test complicated. 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. You are right. I Changed the whole class - look at the new patch set, 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 Done 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. Look at the comment in SecureDeletionHandler.java. 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
