I am reviewing it now. -Sanjay
On 11/27/09 07:12 AM, William Schumann wrote: > Sanjay, > Made 2nd round of changes and issued webrev at: > http://cr.opensolaris.org/~wmsch/bug-6590 > Previous webrev at: > http://cr.opensolaris.org/~wmsch/bug-6590/oldwebrev2/ > > Please find my responses below: > > Sanjay Nadkarni wrote: >> On 11/25/09 01:51 PM, William Schumann wrote: >>> Sanjay, >>> >>> Sanjay Nadkarni wrote: >>>> ... >>>>>> >>>>>> auto_parse.c: >>>>>> 193 and others: >>>>>> if p is NULL, wouldn't this be an error. If so it will not >>>>>> be caught. Similar issues with other parsed values. >>>>> Errors could occur in auto_parse_manifest.c during tag parsing >>>>> that would be ignored. However, these errors would be very >>>>> unlikely, since they would involve a basic breakdown in the >>>>> embedded Python interpreter, which has already been invoked to >>>>> validate the manifest. Since fixing this would involve many >>>>> changes in three different modules, and since this was not >>>>> introduced by iSCSI, I would prefer to file a bug on this and >>>>> handle it later at a more appropriate priority. >>>> >>>> Sorry for not being clear. There are 2 issues here: >>>> >>>> 1. If you truly believe that p != NULL then why not change them to >>>> an assert. >>> p == NULL when there is no value for the tag in the manifest (as >>> well as the Python interpreter errors describe just above). Remember >>> that assert() only has an effect in a debug build. The Python >>> errors in auto_parse_manifest.c could have assert()s so that these >>> failures would cause assertion failures in a debug build. Would >>> that suffice? >> Yup that would ok. > Added asserts for Python failures. >>>> 2. Sorry for not being clear. I was really asking where is the >>>> sematic parsing occurring ? >>> The only semantic parsing with these new values is in >>> td_iscsi.c:parse_lun_num(), and even that doesn't report parsing >>> problems explicitly. Parameters that could benefit from some >>> semantic validation are: IP address, LUN. port could be checked for >>> absurd values. >> Sure..but I what I was getting at was dependency between the various >> elements that are defined in the .rng file. They are marked as >> optional are they really optional ? The xor test indicates that >> there is a dependency/requirement or am I mistaken ? > Not at all. > > I addressed this in detail in my response to Ethan. To summarize, > validation failures currently produce incorrect, misleading, and > confusing console output and useless log file output; however, having > the validation against the schema would be useful since it is done on > the AI server in installadm (when the manifest is submitted), so I > decided to enforce the dependencies via the RNG schema rather than in > the AI code, and improve the output of the validator in the future. > > Thank you, > William >> >> >>>> >>>>>> >>>>>> >>>>>> auto_td.c: >>>>>> 482: Just to be sure..when "if" statement fails, >>>>>> diskiscsiname and diskiscsiip are both either >>>>>> defined or undefined. A comment to that effect would be useful. >>>>>> 492: I would have thought that rootpath[MAXNAMELEN] would >>>>>> be sufficient (i.e. 256) instead of MAXPATHLEN >>>>>> which is 1024. >>>>> According to RFC 3720, the iSCSI name itself can be up to 223 >>>>> bytes. Add to that the maximum length of the IP address, port >>>>> number, protocol, and LUN, plus the "iscsi:" and separator >>>>> characters and this is over 256 bytes. >>>> Okay >>>>>> >>>>>> 539: strncmp(rootpath, "iscsi:", strlen("iscsi:")) would >>>>>> be preferable. >>>>>> >>>> ??? >>> What's the problem here? The requested change on 539 was made in >>> the source and appears in the webrev. >> I believe, I email crossed. Ignore. >>> >>> Thank you, >>> William >>
