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.
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? 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? -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. 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. 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. 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. 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. 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. - 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? - Instead of having "Build completed....." in different places, why not put it at the end of the file? Then, it is just one line. 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. 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 >
