Clay, further follow-up inline.
Dave
On 07/20/10 04:37 PM, [email protected] wrote:
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.
Reading the comments in the modification, this seems misguided. We
don't need to provide for a non-CIDR notation at all; users who for some
reason desire to set this to a single address can always use /32.
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.
Given that argument, I'm frankly disappointed that the judgment wasn't
made to convert the script-masquerading-as-a-C-program that is
do_create_service. Changing 50% of a function and yet leaving it
fundamentally mis-implemented isn't really getting this code where it
should be.
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...
These are infrequently modified, so I'm not sure what benefit you're
anticipating from expending that effort on automation.
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.
See my above comment about when to reimplement.
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?
Never mind.
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.
Most likely we'll look at this as part of the overall package
refactoring in ON - we've talked about creating a base package with
directories that just should be delivered once and everyone can then
depend on.
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.
The timeout value seems much too low. We're defaulted to 900 seconds
since we haven't set it in the current implementation; reducing it by
90% seems much too aggressive based on our fairly limited experience
with the performance aspects here.
Dave
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
