Mike Kolesnik has posted comments on this change.
Change subject: Handle missing/invalid mac address on import Vm/Template
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(8 inline comments)
I put -1 because Sharad is correct and the logic of the can do action is faulty
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 339: // check if mac address is valid
I think this comment is useless, it adds nothing to the readability of the code.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java
Line 183: // check if mac address is valid
I think this comment is useless, it adds nothing to the readability of the code.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java
Line 329: protected void addMissingMacAddress(VmNetworkInterface iface) {
The method name is not clear to me, I would prefer something in the lines of
"fillMacAddressIfMissing" which describes more accurately what this method does.
Line 330: if (StringHelper.isNullOrEmpty(iface.getMacAddress())) {
This method is being called as part of the execute, so it is past validation
already and no need to check that.
Line 336: if (!Regex.IsMatch(iface.getMacAddress(),
VmNetworkInterface.VALID_MAC_ADDRESS_FORMAT)) {
Please don't use Regex since it is compat code.
Instead, use Pattern.compile to compile the regex pattern and keep that in a
constant field.
Please have a look at
http://docs.oracle.com/javase/6/docs/api/java/util/regex/Pattern.html to see
how it's done.
Line 337: AuditLogableBase logable = new AuditLogableBase();
Why log?
The user will get an immediate failure anyway and won't be able to continue, so
i see no reason to convey this information to him twice.
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 756: VM_IMPORT_INTERFACES_WITH_INVALID_MAC_ADDRESS=Cannot improt Vm
${VmName}. It have an interface ${IfaceName} with an invalid MAC address
${MacAddress).
You have a typo - improt should be import.
Besides, I think it is better to use the standard "Cannot ${action} ${type}"
since it will fit both import vm & template (and maybe other cases). Of course
the KEY needs to be something like ACTION_TYPE_FAILED_NETWORK_INTERFACE_INVALID.
Also I think the message would sound better like this:
Cannot ${action} ${type}. The Network Interface ${IfaceName} has an invalid MAC
address ${MacAddress}. MAC address must be in format "HH:HH:HH:HH:HH:HH" where
H is a hexadecimal character (either a digit or A-F, case is insignificant).
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 502: IMPORTEXPORT_IMPORT_INTERFACES_WITH_INVALID_MAC_ADDRESS=Cannot improt
Vm ${VmName}. It have an interface ${IfaceName} with an invalid MAC address
${MacAddress).
At audit log messages it is not a custom to write "Cannot X Y.", only at the
can do action messages.
Also I think the message is redundant since it's blocked by can do action
anyway.
--
To view, visit http://gerrit.ovirt.org/5290
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09dd86a352ecc17e80dceb8c331ec38f4fa96627
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Shahar Havivi <[email protected]>
Gerrit-Reviewer: Sharad Mishra <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
