Liron Aravot has posted comments on this change.
Change subject: engine: Add validation to cpu pinning
......................................................................
Patch Set 4: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java
Line 333: if (returnValue) {
Line 334: VM vmFromParams = getParameters().getVm();
Line 335: returnValue =
isCpuPinningValid(vmFromParams.getCpuPinning(),
Line 336: vmFromParams.getStaticData(),
Line 337: getReturnValue().getCanDoActionMessages());
with current implementation no reason to pass the messages list, the check is
not in "external" class.
Line 338: }
Line 339:
Line 340: if (getParameters().getVm().isUseHostCpuFlags()
Line 341: && getParameters().getVm().getMigrationSupport() ==
MigrationSupport.MIGRATABLE) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Line 276: }
Line 277:
Line 278: // check cpuPinning
Line 279: if (!isCpuPinningValid(vmFromParams.getCpuPinning(),
vmFromParams.getStaticData(),
Line 280: getReturnValue().getCanDoActionMessages())) {
why do you need to pass the messages list? you can just access it from
isCpuPinningValid
Line 281: return false;
Line 282: }
Line 283:
Line 284: if
(!validatePinningAndMigration(getReturnValue().getCanDoActionMessages(),
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmManagementCommandBase.java
Line 72:
Pattern.compile("\\d+#(\\^\\d+|\\d+\\-\\d+|\\d+)(,(\\^\\d+|\\d+\\-\\d+|\\d+))*"
+
Line 73:
"(_\\d+#(\\^\\d+|\\d+\\-\\d+|\\d+)(,(\\^\\d+|\\d+\\-\\d+|\\d+))*)*");
Line 74:
Line 75: /**
Line 76: * Checks that a given CPU pinning string is valid returns
VdcBllMessages.Unassigned if no error was found or the
comment is wrong now..returns boolean.
Line 77: * corresponding error
Line 78: *
Line 79: * @param cpuPinning
Line 80: * String to validate
Line 87: * @return if the given cpuPinning is valid
Line 88: */
Line 89: public boolean isCpuPinningValid(final String cpuPinning,
Line 90: VmStatic vmStatic,
Line 91: List<String> messageList) {
I think that the correct place for this method is in VmHandler, so in case of
need it can be used from other commands as well. if you decide to leave it
here, no need to pass the messages list.
Line 92:
Line 93: if (StringUtils.isEmpty(cpuPinning)) {
Line 94: return true;
Line 95: }
Line 120:
messageList.add(VdcBllMessages.VM_PINNING_DUPLICATE_DEFINITION.toString());
Line 121: return false;
Line 122: }
Line 123:
Line 124: TreeSet<Integer> currPcpus =
parseCpuPinningNumbers(splitRule[1]);
why TreeSet?
Line 125: if (currPcpus == null) {
Line 126:
messageList.add(VdcBllMessages.VM_PINNING_FORMAT_INVALID.toString());
Line 127: return false;
Line 128: }
Line 134: }
Line 135:
Line 136: // can not check if no dedicated vds was configured
Line 137: if (vmStatic.getdedicated_vm_for_vds() != null) {
Line 138: VDS dedicatedVds = getVds((Guid)
vmStatic.getdedicated_vm_for_vds());
use NGuid.getValue instead of the casting.
Line 139: // check only from cluster version 3.2
Line 140: if (dedicatedVds != null &&
Line 141:
dedicatedVds.getvds_group_compatibility_version() != null &&
Line 142:
dedicatedVds.getvds_group_compatibility_version().compareTo(Version.v3_2) >= 0
&&
Line 153: return true;
Line 154: }
Line 155:
Line 156: protected VDS getVds(Guid id) {
Line 157: return DbFacade.getInstance().getVdsDao().get(id);
no need for that..you already should have here getVdsDao() method.
Line 158: }
Line 159:
Line 160: private TreeSet<Integer> parseCpuPinningNumbers(final String
text) {
Line 161: try {
Line 159:
Line 160: private TreeSet<Integer> parseCpuPinningNumbers(final String
text) {
Line 161: try {
Line 162: TreeSet<Integer> include = new TreeSet<Integer>();
Line 163: TreeSet<Integer> exclude = new TreeSet<Integer>();
why TreeSets?
Line 164: String[] splitText = text.split(",");
Line 165: for (String section : splitText) {
Line 166: if (section.startsWith("^")) {
Line 167:
exclude.add(Integer.parseInt(section.substring(1)));
Line 164: String[] splitText = text.split(",");
Line 165: for (String section : splitText) {
Line 166: if (section.startsWith("^")) {
Line 167:
exclude.add(Integer.parseInt(section.substring(1)));
Line 168: } else if (section.contains("-")) {
seems like it will be better to just split and check the results rather then
scan the string twice (contains and split).
Line 169: // include range
Line 170: String[] numbers = section.split("-");
Line 171: int start = Integer.parseInt(numbers[0]);
Line 172: int end = Integer.parseInt(numbers[1]);
--
To view, visit http://gerrit.ovirt.org/10364
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I690b60fcde6bf337058246ef9b1ee24fa3b7220a
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Noam Slomianko <nslom...@redhat.com>
Gerrit-Reviewer: Andrew Cathrow <and...@cathrow.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Noam Slomianko <nslom...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
