Karen Tung wrote: > Hi Jean, > > I few response to your comments inline. > > Jean McCormack wrote: >>> >>> >>> 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. > hmmm...so, perhaps all the optional attributes have to have a default > specified? Not sure. > I guess Jack would know the answer. Can we do a "" as the default, > like nodepath="img_params/user/password". > I know that's meaningless, but at least, it will make the entry look > like the others. Sure. That makes sense.
Jean >>> >>> 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. > I don't really care either way, but just thought to mention it. To > me, it seems to make > more sense that way. Maybe somebody else will have opinion and their > supporting reasoning on this. >>> >>> 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. > That's kinda odd unexpected behavior. To me, None means it is not > specified. "" means it is > specified, but it is just empty. If it can't be empty, the validator > module should > catch it, and report the error then. That way, the app won't have to > do the duplicate check. > What do you think? We leave the check in the way it is right now, and > file a bug to fix the > validator, and then, remove the checks? >> >>> >>> 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. > OK >>> >>> 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. > Same comment as the checkpoint name one above. >> >>> >>> 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. > I am OK with leaving it as it is right now, then. >>> >>> - 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 > Oh ok. I was expecting that it will also be the same table. > But I guess the way you do it in DC_checkpoint.py is also OK. > > Thanks, > > --Karen
