Nirmal,
new webrev looks good to me.
Thank you for updating the CR.
Jan
On 08/21/12 03:22 PM, Nirmal Agarwal wrote:
Hi Jan,
Please find the updated webrev as per discussion.
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7191720-rev1/webrev/
Also, I have updated the CR in bugster with the information.
Thanks,
Nirmal
On 08/21/12 18:05, Jan Damborsky wrote:
On 08/21/12 01:40 PM, Nirmal Agarwal wrote:
Hi Jan
Thanks for the review. Please find my comments inline.
On 08/21/12 14:33, Jan Damborsky wrote:
Hi Nirmal,
I have one generic comment related to this fix. Looking at slim_source,
there seems to be another implementation of hostname validation
algorithm in
usr/src/cmd/gui-install/src/user_screen.py:validate_hostname().
I am wondering if it may make sense to eliminate this duplication.
Given the current state of things, handling that as separate CR
would be fine.
I will raise a CR to eliminate the duplication.
Thank you.
Please see below for code review comments.
Thank you,
Jan
232-233 - I am wondering if we really want to eliminate one
character hostnames,
as looking at Drew's comment in the CR, nothing indicates there that
hostnames should be at least two characters long.
As we discussed off line, it seems one character hostnames should be
allowed,
as they are not ruled out per hosts(4) man page, as well as they are
allowed by other implementations (GUI installer, legacy sysid tools).
240-242 - Along the same lines, I am wondering if eliminating
hostnames containing only digits is appropriate, as looking at
Drew's comment
in the CR, it seems that those are allowed.
The above checks are as per man page of hosts.
Yeah, looking at that man page, I can see it says that hostnames
containing
just digits are disallowed:
...
Host names must not consist of numbers only. A host name
must contain at least one alphabetical or special character.
...
I may recommend to update CR with that information.
Thank you,
Jan
Thanks,
Nirmal
On 08/21/12 09:24 AM, Nirmal Agarwal wrote:
Hi all,
Can I please get 2 code reviewers for CR 7191720.
7191720 <http://monaco.us.oracle.com/detail.jsf?cr=7191720> S11
sysconfig doesn't allow . in the hostname
webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7191720/webrev/
Pep8 is clean.
Pylint output is unchanged.
Testing :
I have tested the code with the following inputs.
--> hostname can contain "."
--> hostname should not start or end with "." or "-"
--> hostname should contain atleast 1 alphabet or '.' or '-'.
Thanks,
Nirmal
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
