Alissa Bonas has posted comments on this change.
Change subject: engine: Host name validation cleanup
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
Line 287:
addCanDoActionMessage(VdcBllMessages.VDS_CLUSTER_IS_NOT_VALID);
Line 288: returnValue = false;
Line 289: } else {
Line 290: VDS vds = getParameters().getvds();
Line 291: String hostName = vds.gethost_name();
The extraction of the hostname is better to be done couple of lines below -
next to the if statement that checks validHostName - it's relevant there. and
not near checking vds name..
Line 292: if(!validateVdsName(vds)) {
Line 293: returnValue = false;
Line 294: } else if (!ValidationUtils.validHostname(hostName)) {
Line 295:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_INVALID_VDS_HOSTNAME);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsCommand.java
Line 48: _oldVds.getstatus())) {
Line 49: if
("".equals(getParameters().getVdsStaticData().getvds_name())) {
Line 50:
addCanDoActionMessage(VdcBllMessages.VDS_TRY_CREATE_WITH_EXISTING_PARAMS);
Line 51: }
Line 52: String hostName =
getParameters().getvds().gethost_name();
same comment regarding placing this variable couple of lines below like in the
"AddVDS" command.
Line 53: if(!validateVdsName(getParameters().getvds())) {
Line 54: returnValue = false;
Line 55: } else if (_oldVds.getstatus() !=
VDSStatus.InstallFailed && !_oldVds.gethost_name().equals(hostName)) {
Line 56:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_HOSNAME_CANNOT_CHANGE);
Line 49: if
("".equals(getParameters().getVdsStaticData().getvds_name())) {
Line 50:
addCanDoActionMessage(VdcBllMessages.VDS_TRY_CREATE_WITH_EXISTING_PARAMS);
Line 51: }
Line 52: String hostName =
getParameters().getvds().gethost_name();
Line 53: if(!validateVdsName(getParameters().getvds())) {
It is safer to extract the VDS first and check it's not empty, before sending
straight getParameters.getVDS to the method - it could end up here with
NullPointerException - easier to catch it beforehand.
Line 54: returnValue = false;
Line 55: } else if (_oldVds.getstatus() !=
VDSStatus.InstallFailed && !_oldVds.gethost_name().equals(hostName)) {
Line 56:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_HOSNAME_CANNOT_CHANGE);
Line 57: returnValue = false;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsCommand.java
Line 196:
getReturnValue().getFault().setMessage(returnValue.getVdsError().getMessage());
Line 197:
getReturnValue().getExecuteFailedMessages().add(returnValue.getVdsError().getMessage());
Line 198: }
Line 199:
Line 200: public boolean validateVdsName(VDS vds) {
Since the method is just checking the validity of the name, it is enough to
send just the name to it (same as done with host name in
ValidationUtils.validHostname(hostName))
And since this method returns a boolean, it's better to call it isVdsNameValid.
Line 201: final int maxVdsNameLength = Config.<Integer>
GetValue(ConfigValues.MaxVdsNameLength);
Line 202: final String vdsName = vds.getName();
Line 203: if (vdsName == null || vdsName.isEmpty()) {
Line 204: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_NAME_MAY_NOT_BE_EMPTY);
--
To view, visit http://gerrit.ovirt.org/11189
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5288bfd8020c8339d9d8870c6551b63351be79e4
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
