Hi Joe,
Thank you for the continual clean-up help. My comments on nit
acceptance or a glimpse into the brain o' Clay as to why I disagree.
Thank you,
Clay
On Mon, 2 Aug 2010, [email protected] wrote:
[snip...]
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/installadm-common.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Issue:
------
I have little to add regarding validate_networks/calculate_net_address
beyond the comments from Dave and Ethan.
I am concerned about the complexity of this logic and migrating it
to Python. Perhaps the best thing might be to request enhancement
to exiting network adm tools to have them provide this info.
I will look in to this as it would be valuable for the ISIM
projec.
I believe this will be much easier and more compact to do in Python than
it was in shell. Much of this code dose what Python natively does with its
set data-type. Further, I have an example Python script which uses the
getifaddrs(3SOCKET) functionality via the CTypes interface so the
interfacing and processing of ipadm(1) should be unnecessary. Also, some
of what was here cleaned-up or added for DHCP work is already roughed out
or fully available via installadm_common.py too.
Question:
---------
Had you investigated using ipadm show-if as we talked about?
I don't see it being done. Was it not viable?
Yes, however, I need the data provided by ipadm show-addr to see the IP
level details which are necessary to support the multi-homed work show if
only appears to provide link-level details which are not all that I need.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-dhcp.sh
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit:
---
These comment doesn't seem to read properly.
104 # value will cause the for loop below to split on
I've tried to clarify this comment with the following wording:
# see if we have a BootFile option
# this is necessary as colon's embedded i
# value will cause the for loop below to
# the value instead of on the DHCP option
126 # next, check to see if this option is set and
127 # if so, set differently, then save in a
128 # instructions
129 # for the user
Yes, this is a case of me's speaks good engrish I think; how about:
# next, check to see if this option is set and
# if set differently, then save instruction
# for the user
This is for a DHCP option being set, and set to something different than
we desire, then we need to tell the user (opposed to just attacking their
DHCP server macros).
Nit/issue:
---------
This is a general comment which could be applied throughout the code.
Perhps this is a style thing but I think having comment describing
blocks of code are more valuable then line by line comments.
Admittedly, I think this is stylistic. I appreciate large overall method
block describing the layout of a code block to understand what the
architecture was but I like a blow-by-blow why someone is doing something
-- especially in shell scripting where there's a million things going on
and not much is explicit.
I think this could benifit from being compressed from:
126 # next, check to see if this option is set and
127 # if so, set differently, then save in a
128 # instructions
129 # for the user
130 match_macro_and_option $name ${optstr%%=*} \
131 ${optstr#*=}
132 if (( $? == 1 )); then
133 # add a space and option name to
134 # $collision_opts
135 collision_opts+=" ${optstr%%=*}"
136 # add a space and option value to
137 # $collision_opts
138 collision_opts+=" ${optstr#*=}"
139 continue
140 fi
141 optstr=${optstr/\$boot_file/$boot_file}
142 $DHTADM -M -m $name -e $optstr
143 IFS=":"
144 done
145 IFS="$oifs"
146 # print colliding options if any were found
147 [[ -n $collision_opts ]] && print_macro_opt_collision \
148 $name "$collision_opts"
To:
# Generate a space separated list of any matching options. Change
# as needed and save as printable instructions for the user.
match_macro_and_option $name ${optstr%%=*} \
${optstr#*=}
if (( $? == 1 )); then
collision_opts+=" ${optstr%%=*}"
collision_opts+=" ${optstr#*=}"
continue
fi
optstr=${optstr/\$boot_file/$boot_file}
$DHTADM -M -m $name -e $optstr
IFS=":"
done
IFS="$oifs"
if [[ -n $collision_opts ]]; then
print_macro_opt_collision $name "$collision_opts"
Despite authoring this, I can't tell you without the comments what
${optstr#*=} is supposed to be (imagine if this was misbehaving) or if it
was intentional to have a space in both updates of collision_opts.
The line-by-line commentary is something I used to appreciate when
debugging large Perl scripts in another life. The line by line rationale
was a sanity check (is this intentional or a typo/bug/mis-feature) I think
for C and Python, it's far less useful - partially due to code blocking
and partially due to language style difference. (E.g. string processing in
Python is much more deliberate than the hacking on strings to get what's
desired in shell.)
Issue:
-----
line 608 down.
Perhaps not worth changing at this point but instead of the
sequence of if/elif a case would be a better choice.
Yup; this is the format all the installadm code was initially written
with. I think optparse or argparse in Python would be much better still.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/cmd/installadm/setup-image.sh
and other .sh files
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Nit/Style thing.
Why change these to use all the ${CMD} usage?
Why not rely on $PATH?
It was already somewhat established in places to use $CMD substitution so
I continued throughout.
See:
http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips
http://hub.opensolaris.org/bin/view/Community+Group+on/shellstyle
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
