Thanks Nirmal -- looks good.
ginnie
On 08/21/12 09:45 AM, Nirmal Agarwal wrote:
Hi Ginnie,
Thanks for the review. I have incorporated the changes and updated the
webrev.
webrev:
https://cr.opensolaris.org/action/browse/caiman/nirmal27/7191720-rev2/webrev/
Thanks,
Nirmal
On 08/21/12 20:38, Virginia Wray wrote:
Hi Nirmal --
The copyright needs to be updated.
Also, the comment at line 239 -240 is a little awkward.
The Hostname must contain at least one "
"alphabet.....
I think it should be "one alphabetic character", or something
similar.
thanks,
ginnie
On 08/21/12 07:22 AM, 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
