Evan Layton wrote: > Ethan Quach wrote: >> >> >> Sundar Yamunachari wrote: >>> Ethan Quach wrote: >>>> Sundar, >>>> >>>> Thanks for the comments .... >>>> >>>> >>>> Sundar Yamunachari wrote: >>>>> Ethan, >>>>> >>>>> *usr/src/cmd/installadm/create-client.sh: >>>>> >>>>> *199: Just a nit... The comment doesn't sound right. >>>> >>>> # If IMAGE_SERVER is not empty, this means it was passed in by the >>>> # user. So if its not empty, check that its equal to the local system >>>> # since we don't yet support a remote system being the image server. >>> I was thinking more like the wording "If IMAGE_SERVER was passed in, >>> check that its the local machine". The "its" here means "it is"? >> >> Ah okay :-) >> >> Webrev is updated, and merged with the 7218 push. >> >> Also updated setup-dhcp.sh:309-327 to only create the network >> table in the DHCP server with a router if the router found is for >> the network we're configuring. If not, its not used, and a message >> is displayed. >> >> >> Fyi, retested everything after doing a post 7218 convert, >> (which worked quite well, though it took enabling one service >> for all of them to get kicked online.) Also filed 7982 which >> seems to have been broken by 7218. > > Thanks again for catching these issues! > > Your changes look fine to me.
Thanks for the review. -ethan > > -evan > >> >> >> thanks, >> -ethan >> >>>> >>>>> >>>>> *usr/src/cmd/installadm/installadm-common.sh:* >>>>> >>>>> 102: Check whether ipaddr is non-null after line 98. If it is >>>>> null, this will result in syntax error. >>>> >>>> Will do. >>>> >>>>> 122: Validate ipaddr and netmask >>>> >>>> Will do. >>>> >>>>> 123: Can you add some comment to indicate what bitwise_and does >>>>> and why you are doing it? >>>> >>>> bitwise_and() ands together the lower four bits of the two >>>> decimal values passed in, and returns the result as a decimal >>>> value. I'll add this comment. >>>> >>>> Fyi, this whole get_network() routine was taken from an existing >>>> script that we deliver with jumpstart servers, "chkprobe". >>>> >>>>> >>>>> *usr/src/cmd/installadm/installadm.c* >>>>> >>>>> Are these changes part of your bug fix? It looks like saving >>>>> service data is moved up. >>>> >>>> Yes, they are part of this bugfix. I've moved registering the service >>>> up above dhcp-server configuration so that upon any dhcp-server >>>> configuration failure, one can cleanly do a "delete-service". >>>> >>>> It makes sense to move it up there anyway because at that point, >>>> we've created the target image and have set up on the service >>>> on the server already. >>> okay. I wanted to make sure that it is deliberate and not webrev >>> problem. >>> >>> - Sundar >>>> >>>>> >>>>> *usr/src/cmd/installadm/setup-dhcp.sh* >>>>> >>>>> 63: Make sure that IP address is non-null >>>> >>>> Will do. >>>> >>>> >>>> thanks, >>>> -ethan >>>> >>>>> >>>>> - Sundar >>>>> >>>>> >>>>> Ethan Quach wrote: >>>>>> Can I get a review for these blockers. >>>>>> >>>>>> >>>>>> Webrev: >>>>>> ------------ >>>>>> http://cr.opensolaris.org/~equach/webrev.5589.7797.7226/ >>>>>> >>>>>> Defects: >>>>>> ------------ >>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5589 >>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7797 >>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=7226 >>>>>> >>>>>> >>>>>> >>>>>> thanks, >>>>>> -ethan >>>>>> >>>>>> _______________________________________________ >>>>>> caiman-discuss mailing list >>>>>> caiman-discuss at opensolaris.org >>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> >>> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
