Alok, The 'cdiff' and 'udiff' differences do not include whitespace changes. 'wdiffs' and 'patch' reveal these. http://cr.opensolaris.org/~wmsch/bug-4246/usr/src/cmd/auto-install/auto_parse_manifest.c.patch William
Alok Aggarwal wrote: > Looks fine now, thanks for making the changes. > > For some reason webrev seems to think that > 38 lines have changed in auto_parse_manifest.c > when far fewer have actually changed .. hmm. > > Alok > > On Fri, 6 Feb 2009, William Schumann wrote: > > >> Alok, >> Implemented all of your suggestions and reissued webrev. Comments below. >> >> Alok Aggarwal wrote: >> >>> On Thu, 29 Jan 2009, William Schumann wrote: >>> >>> >>> >>>> Alok, >>>> Responses below. >>>> >>>> Alok Aggarwal wrote: >>>> >>>> >>>>> On Tue, 27 Jan 2009, William Schumann wrote: >>>>> >>>>> >>>>> >>>>>> Passwords should be encrypted in manifests, not by the installer code. >>>>>> >>>>>> The basic fix was simple - remove encryption calls from the code - but >>>>>> it was noted that there could be XML entities encoded in the password, >>>>>> so XML entities will now be decoded. The XML decoding is basic and has >>>>>> some shortcuts, such as decoding only 1-byte numeric values. It is >>>>>> expected that the AI manifests will soon be restructured and then >>>>>> should >>>>>> be parsed with established parsers, such as in libxml2. >>>>>> >>>>>> Bugs fixed along the way: >>>>>> - values with spaces were being truncated at the first space >>>>>> - apostrophes used as value delimiters were not working >>>>>> - some typos with keyword name not matching intended storage variable >>>>>> name >>>>>> >>>>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=4246 >>>>>> http://cr.opensolaris.org/~wmsch/bug-4246/ >>>>>> >>>>>> >>>>> auto_parse.c: lines 581-583: What happens if there >>>>> are multiple blank spaces either one after the other >>>>> or separated by tokens? >>>>> >>>>> >>>> Spaces are compressed to a single space - fixed. >>>> >>>> >>> Okay. >>> >>> >>> >>>>> auto_parse.c: decode_xml_entities(): This code looks >>>>> complicated. Given that the SC manifest is going to eventually move out >>>>> to SMF enhanced profiles, I do >>>>> wonder whether it's worth the effort to write this code >>>>> that's going to be thrown away once we move over to SMF >>>>> enhanced profiles. >>>>> >>>>> >>>> 90 lines of code here. The effort would be in reviewing it and dealing >>>> with possible bugs. >>>> >>>> >>> Maintainability is the other area that would need more >>> effort than usual. >>> >>> >> Given that fully functional XML parsing code will be used in the near >> future, I think that supporting the use of XML elements in the SC manifest >> can wait until then. Removed decode_xml_entities() XML parsing code, filing >> it for possible use later. >> >>> auto_parse.c: line 627: I don't think we want to do gratuitous truncation >>> (say, if it's a password value). >>> A failure is more appropriate. >>> >>> >> Added failure path for bad passwords. >> >>> auto_parse.c: line 667: shouldn't this be zero unless >>> the code didn't catch an error condition else where? >>> >>> >> Traced bad errno to auto_parse_manifest.c where a malloc was given a length >> of -1 when optional XML element was missing. Fixed and removed nits. >> >>> auto_parse.c: line 691: I don't think this should get >>> logged to the console, logging to the install_log should >>> be adequate. >>> >>> >> Removed. >> William >> >>>> Another XML parsing problem is that it is assumed that the SC value is on >>>> the same line as the "propval" keyword and AI will fail if it isn't. >>>> >>>> Do you know a time frame for moving to SMF enhanced profiles? >>>> >>>> >>> I don't know what the plans are in this area. >>> >>> >>> >>>>> Lastly, which cases did you test while testing out these >>>>> changes? >>>>> >>>>> >>>> Tried values: >>>> - single, multiple spaces >>>> - empty >>>> - all possible (5) XML entities; XML decimal, octal encodings >>>> - invalid XML entities, missing terminator >>>> - XML entities adjacent, at beginning of line, at end of line >>>> - value longer than VALUE_SIZE (added warning on overflow) >>>> >>>> >>> Thanks. >>> >>> Alok >>> _______________________________________________ >>> 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 >
