Arik Hadas has posted comments on this change.
Change subject: frontend: VM Linux Boot options aren't parsing properly
......................................................................
Patch Set 1: (4 inline comments)
....................................................
Commit Message
Line 3: AuthorDate: 2013-02-25 13:08:14 +0100
Line 4: Commit: Frantisek Kobzik <fkob...@redhat.com>
Line 5: CommitDate: 2013-02-27 13:19:25 +0100
Line 6:
Line 7: frontend: VM Linux Boot options aren't parsing properly
I think that "frontend" is not included in the template of commit messages, so
using it might cause missing this patch when making queries. why not using
"webadmin"?
Line 8:
Line 9: This patch adds checking Linux Boot options in the Run Once dialog.
Line 10:
Line 11: Change-Id: I23bd3ee3677c9bdbfacf1cf341dc1d97fd57c65e
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/RunOnceModel.java
Line 627: String kernelPath = (String) getKernel_path().getEntity();
Line 628: String initrdPath = (String) getInitrd_path().getEntity();
Line 629: String kernelParams = (String)
getKernel_parameters().getEntity();
Line 630:
Line 631: if ((kernelParams.length() > 0 || initrdPath.length() >
0) && kernelPath.length() == 0) {
how about removing the part that replace null strings with empty strings (lines
607-618) and in 631 asking whether the strings are not "isNullOrEmpty" ?
Line 632: boolean kernelParamInvalid = false;
Line 633: boolean inetdPathInvalid = false;
Line 634:
Line 635: if (kernelParams.length() > 0) {
Line 629: String kernelParams = (String)
getKernel_parameters().getEntity();
Line 630:
Line 631: if ((kernelParams.length() > 0 || initrdPath.length() >
0) && kernelPath.length() == 0) {
Line 632: boolean kernelParamInvalid = false;
Line 633: boolean inetdPathInvalid = false;
I'm not sure I understand why this local flags are needed.. is there a reason
to use the flag kernelParamInvalid over getKernel_parameters().getIsValid() ?
the only case that it will return different result is when the kenel-parameters
string has trimming characters - in that the flag will be off although the
input is invalid, is that the expected behaviour?
Line 634:
Line 635: if (kernelParams.length() > 0) {
Line 636: getKernel_parameters().setIsValid(false);
Line 637: kernelParamInvalid = true;
Line 643:
Line 644: String msg =
Line 645: ConstantsManager.getInstance()
Line 646: .getMessages()
Line 647: .invalidPath(kernelParamInvalid ?
ConstantsManager.getInstance()
I would prefer to extract lines 645-655 to private method that return that
string or saving the ConstantsManager.getInstance().getConstants() in a local
field in order to make it shorter, but I don't mind to keep it that way if you
don't agree
Line 648: .getConstants()
Line 649: .kernelInvalid() : "",
//$NON-NLS-1$
Line 650: kernelParamInvalid &&
inetdPathInvalid ? ConstantsManager.getInstance()
Line 651: .getConstants()
--
To view, visit http://gerrit.ovirt.org/12494
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23bd3ee3677c9bdbfacf1cf341dc1d97fd57c65e
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
