Hi Dave,
Thank you for the review of my code! My comments in-line and
webrevs at:
Full:
http://cr.opensolaris.org/~clayb/webrev.multihomed/dave
Differential
(from Joe's code-review response before you):
http://cr.opensolaris.org/~clayb/webrev.multihomed/dave/diff
Thank you,
Clay
On Fri, 16 Jul 2010, Dave Miner wrote:
[snip...]
Clay, I've tried to mostly avoid repeating other comments already provided by
others.
Dave
ai_sd.py
401: "tuple"
Thank you
405: s/record/option/. Also avoid contractions (they're l10n-hostile, though
I note we're not internationalized here...). Better would be "Unable to
resolve BootSrvA option from DHCP server: %s"
Wow, I normally try to avoid contractions for that reason and have them in
a lot of places. I think I have removed them all. Better wording too.
installadm-common.sh
41: the use of gnu sed can be trivially replaced with nawk and thus not
introduce additional dependencies, but see next comment.
Yes, this has been replaced with ksh93 native string operations
483-628: The scheme used here is overly complex, as you don't need to
compare addresses to ranges. All that should be needed is to compute the
network address for each SMF exclude_net entry and apply the same prefix to
each interface address to calculate its network address. If they're equal,
then they're the same network; if not, then they're not.
Unfortunately, as both just an IP address or a CIDR notation network may
be provided I think I need to compare ranges. For example, as a
net_address_v4 SMF property can not be empty, to allow all networks, I
pre-populate the networks property with 0.0.0.0/0 and the exclude_networks
property to false.
499: "beginning"
Thank you
518-9: comment doesn't parse
Gone as the logic has been cleaned up and changed a bit per Joe's request
installadm.c
488: please use one of the existing C interfaces to enumerate interfaces
Unfortunately, there was more intended than just enumerating interfaces.
Please see my response to Darren as to the list of benefits and drawbacks
to the choice of using a shell-script function here. I think a C approach
would be a potential vector for bugs and would be quite redundant.
installadm.h
164: How about "The service %s already exists" (and make a similar change to
166).
Sure, change affected for the following which follow a similar format:
168, 170, 192, 202, 204, 206
server.xml
The reformatting is a major step backwards in readability. Please reverse.
Will do, but I'm look to see if anyone has an automated way to format our
XML files as by-hand to what looks good is a rather non-automated
process...
setup-dhcp.sh
79: "preferably"
Thank you
92-124: This would be a fair bit simpler if the design were for the macro to
be assembled in an array and then converted to a full definition only in the
case of an overwrite or printing it out. Then you'd be able to just invoke
dhtadm without pulling the macro definition you just assembled back apart.
I agree. Unfortunately, arrays can't be passed into a ksh function in any
way I can find.
I could modify the callers to add_macro() to use an array and then pass in
a string of the collapsed array and another string with array value
offsets and repack it into an array?
However, I think this is something best left to when this code
makes it into a more powerful language like Python where these objects can
just be made first class data types and passed around easily.
141: "need"
Thank you, my code shouldn't sound like a lolcat wrote it.
208: this comment isn't accurate, there's a bunch of macro updates that can
happen rather than an add or printing something out
I'm not sure what you mean here. The following block of code (on lines
211-251) constructs a macro value which is eventually handed off to print
or add_macro(). Clearly, something is not obvious in this comment; how can
it be improved?
496: "escaped"
Thank you
setup-sparc.sh
226: we probably should be delivering /etc/netboot in a package; not clear to
me what file mode it'll end up with and so on.
Yes, delivering it in a package would be good. If it were with
wanboot-cgi, it would mean that when the consumer of /etc/netboot
(wanboot-cgi) is removed from the system so it /etc/netboot and its
structure. However, this belief might need to be changed when ISIM causes
/etc/netboot to be used more widely. Please let me know where you think
this should fall and I can remove the mkdir(1) when the plan is in place.
net-fs-root
42-3: What's the justification for going to these timeout values?
Very good thing that should be documented in the code too! No, these are
wild guesses.
The main reason for them is that if one is doing a svcadm refresh on the
service or otherwise bumping the AI setup when a client is booting, the
client would not previously gracefully recover.
The intention here is to provide a 50 second window where the client will
return to downloading and resume where it left off, if Apache is upset
some how.
We could have TRIES=0 to forever keep retrying but this makes debugging
difficult, as one can not ^C out of the wget to get to console-login, if
wget simply loops forever. The TIMEOUT value is only for a DNS lookup
timeout, connect timeout and stalled read timeout. As such, it should not
impact a working but slow link.
57: BOOTFILE is unused
Thank you, it is.
_______________________________________________
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
