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
>
