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. >> >> 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
