Jan Damborsky wrote: > Hi Ethan, > > > Ethan Quach wrote: >> Jan, >> >> Thanks for the review... >> >> >> jan damborsky wrote: >>> Hi Ethan, >>> >>> please see my comments below. >>> >>> Thank you, >>> Jan >>> >>> >>> I have general comment for DC changes. Since installadm(1M) >>> tools will take care of populating auto_install directory if it >>> doesn't exist, does DC need to take care of it as well ? >> >> The original intent was to do it only in DC so that installadm >> has to do less work, but then I realized to support older build >> images, installadm should also do it as a back up, since older >> images won't have the directory there. > > I see - thank you for clarifying this. > >> >> So the fact that installadm does it was supposed to be >> secondary. In the future, if we change our scripting to just >> loopback mount the AI iso instead of cpio'ing it, (an idea we've >> thrown around before), it'd be better that these images have >> them there since the imagepath won't be writable, and we'd >> still want this support for those images. > > This is a good point - that will mean that installadm part > which takes care of populating auto_install subdirectory would > be removed at that point, since no writes will be possible > to AI image area - might it be valid assumption ?
Yes. >> The alternative would be to just copy off the needed files >> away from the imagepath altogether, into some per-service >> configuration location on the server. But if we every wanted >> to change to do this, it wouldn't be incompatible with either >> change that we do now. > > I see - I think that having this stuff as part of AI image > is better at this point, since schema to be picked up > for validation is determined directly by AI image to be booted > and indirectly by install service. > That means if different AI image was associated with given install > service, it wouldn't be needed to replace that per-service > configuration location with files from newly associated AI image. Yes. >>> >>> setup-image.sh >>> -------------- >>> >>> generic comment - stderr of CLI used is redirected to /dev/null >>> (e.g. 310, 327, ...). >>> I am thinking if it might be better it goes to the console, >>> since if something goes wrong when those commands are invoked >>> it might be easier to debug potential issue. >> >> It's kinda moot. These scripts are all exec'ed from installadm.c >> so none of these are currently seen anyway. But you're right, >> it probably makes sense for some places to allow stderr to go >> to the console, but not all. I'll update the calls at lines 317, 327, >> 334, 345. That sounds reasonable? > > Why not to allow stderr going to console for other commands ? > I mean lines 310, 331, 335, 343, 347. I'll change them all. thanks for the review. -ethan > >> >> One thing I'm failing to capture is the return code at line 340. >> I'll add this. Also, I'm going to redirect 340 to /dev/null since >> we don't really want to see that ugly "records in/records out" >> output. (If we ever call these scripts directly) > > These are good suggestions - thanks for making those changes ! > Jan > >
