Karen, Thanks for the review. Comments inline.
Karen Tung wrote: > Hi Jean, > > Your webrev list doesn't include any change to ai_x86_image.xml. I > believe that file > also need to be updated with changes similar to the slim_cd.xml file. You're correct. Done. > > Here are my comments about files that's on the webrev: > > usr/src/cmd/distro_const/DC-manifest.defval.xml > > -lines 223-225: You didn't specify a default value here. Doesn't > everything need a default value > to be specified in the default section? The problem I ran into was that since it's an optional attribute, if I didn't list this in the defaults file I received an error. (Validation?) Since I'd like the default to be Executing <script> which you can't really put into the defaults file I put nothing. It worked. So I'll have to say it's not required. Jack may have more to say on this. > > usr/src/cmd/distro_const/DC-manifest.rng > > -General: the build area is now something associated with the whole DC > app, not just > the image. Should we move it to the "distro_constr_params" section > instead of > leaving it in the "img_params" section? Not sure. To me the line is very thin as to the difference between these two sections. > > -line 440: Nite: I know you are not going to fix the problem with the > mirror in this putback, so, > perhaps you shouldn't change this comment yet. Ack you're correct. The comment is back. > > usr/src/cmd/distro_const/DC_checkpoint.py > > -lines 688-692: The checkpoint_name for each finalizer scripts is > required. You already > specified this requirement in the DC-manifest.rng. The manifest > checking module would > have already enforced that requirement, you don't need to check it here. Not true. If you do this in the manifest file: <checkpoint name=""> Then it validates OK but the read code will return None. > > usr/src/cmd/distro_const/DC_ti.py > > -The function DC_create_subdirs() is called in multiple places in the > DC_create_build_area() > function. Since the DC_create_subdirs() function can handle both ZFS > and UFS mount points, > I think it would make the code much easier to read if you were to have > DC_create_build_area() be focused on determining using ZFS or UFS, and > enable checkpoint or not.. > Set up all the mountpoints and stuff. Then, at the end of > DC_create_build_area(), call > DC_create_subdirs() instead of calling it all over the place. I'd like to leave this for now since another bug I'm working on (the zfs datasets created before execution starts bug) addresses this and changes it along with other changes. > > usr/src/cmd/distro_const/DC_tm.py > > -Not quiet sure which bug the change in line 179 is fixing. > Would "alt_url" be None, if it is part of the "add_repo_url_list"? > I guess I am confused about what condition this check is trying to > detect. Like the checkpoint name. The user leaves the value as "" which validates but then becomes none and is unusable. > > > usr/src/cmd/distro_const/DefaultsModule.py > > -I think the out_img_path() function is also not used after your > putback of the work area stuff. > So, it can be deleted. Nice catch. Deleted. > > usr/src/cmd/distro_const/distro_const.py > > -Lines 55-64: This is code is for verifying the command line. Would it > be better to have > this preliminary check of the command line options as a separate > function, > and call it before DC_get_manifest_server_obj(). That way, it will be > very > clear as to where commandline args are checked initially. Then, > in DC_parse_command_line(), we can skip this initial check, and just > check > for the resume/pause step stuff. I think this makes the code much > easier to read. Can't. There's a complicated series of interdependencies here. I'm actually making this cleaner with the zfs dataset bug too. Basically, in order to do the -r -p checks you need to know the build_area and be able to check if you can checkpoint or not. To do that, you need the manifest server obj. > > - So, the "chart" that displays what steps are available to be resumed is > only displayed when people specify "-l"? What if they specify a step to > resume and it is not valid. What will get displayed? You must specify an earlier step to resume at. Valid steps to resume from are: <list of steps you can resume from> So basically a more specific version of the -l > > - Instead of having "Build completed....." in different places, why > not put it at the end > of the file? Then, it is just one line. Good idea. Done. > > usr/src/cmd/distro_const/finalizer_checkpoint.py > usr/src/cmd/distro_const/finalizer_rollback.py > > -For both finalizer_checkpoint.py and finalizer_rollback.py, I think > it would > be a good idea to check for a minimum required length for the arguments. OK. I can do minimum, just not exact. jean > > Thanks, > > --Karen > > Jean McCormack wrote: >> I'd like Jack, when he gets back, and Karen to look at this: >> >> Webrev: >> http://cr.opensolaris.org/~jeanm/DC_bugs/ >> >> >> bugs fixed: >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3357 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3359 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3467 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3555 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3556 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3557 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3560 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=3647 >> >> Jean >> >> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> >
