Hi Clay, On 03/04/10 08:53, Clay Baenziger wrote: > Hi Ethan, > We already ensure all manifest files end in .xml[1]. Also, > delete-manifest already checks if the manifest requested is not found > in the AI DB, if it's listed ending with .xml[2].
I'm not questioning if the current implemented code already does these checks. My comment is really asking if we should relook at that to see if there is a better approach. What I'm seeing here (and correct me if I'm wrong) is that we take a user provided <string value>, and base an internal filename from that. But the internal filename is generated conditionally. Sometimes it'll end up being <string value>.xml and other times its just <string value> Then we have subsequent code chase that conditional when something needs get back out that user <string value> because we have to extrapolate it from the internal filename. Is there perhaps a better (more direct) way for that subsequent code to get at <string value> ? > I think your pathological case is valid as if someone names their AI > manifest "foo.xml.xml" it will be stored that way but the admin. would > have to have some bizarre reason. The reasons are irrelevant. Its a 'text string' value, and the user has no notion, nor should they be impacted by, what we do with it internally. thanks, -ethan > > Thank you, > Clay > > [1]: publish-manifest > http://src.opensolaris.org/source/xref/caiman/slim_bp_installgrub/usr/src/cmd/ai-webserver/publish-manifest.py#692 > > > > [2]: delete-manifest > http://src.opensolaris.org/source/xref/caiman/slim_bp_installgrub/usr/src/cmd/ai-webserver/delete-manifest.py#64 > > > > On Wed, 3 Mar 2010, Ethan Quach wrote: > >> John, >> >> This fix seems to fix the typical cases of manifest names, but there >> are out-lier cases that would still seem >> to be problematic. With this fix, manifests with name fields >> "foo" and "bar" will now show up as "foo" and "bar" in the >> list output; but manifests with name fields "foo.xml" and >> "foo.xml.xml", show up as "foo" and "foo.xml" respectively. >> Its the out-lier cases, but that seems still broken. >> >> As per your note in Comment 1, >> >> # everywhere we expect manifest names to be file names so ensure >> # the name matches >> if not name.endswith('.xml'): >> name += ".xml" >> return name >> >> Removing it would require additional changes within >> publish-manifest and >> probably the delete-manifest. >> >> >> is a subsequent bug being filed to fix that? Because really, >> that seems to be the underlying culprit of this >> inconsistency. We need to be consistent and just append >> '.xml' to all name fields as-is (whether or not they already >> end in .xml), or not append for all. >> >> >> thanks, >> -ethan >> >> >> >> On 02/26/10 16:53, John Fischer wrote: >>> All (especially Clay and Ethan), >>> >>> Here is another webrev that I need reviewed: >>> >>> http://cr.opensolaris.org/~johnfisc/12724-manifest-names >>> >>> This is for: >>> >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=12724 >>> >>> The AI Database code stores manifest names as filenames (with the >>> extension .xml). These change adds an optional parameter >>> (return_fnames) to the getManNames function in the AI_database.py >>> with the default value of True. Thus the code generates the original >>> returned filenames (manifest-name.xml). The installadm list >>> subcommand code sets this optional parameter to False and gets the >>> .xml extension stripped off (the manifest name instead of the >>> filename). >>> >>> I believe this approach is better then changing the internal database >>> storage of the manifests as the rest of the webserver code assumes the >>> manifest filename and not the manifest name. >>> >>> Thanks, >>> >>> John >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
