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.
>
Thanks for the explanation. I have also reviewed the updated changes
and I am okay with a minor nit in td_iscsi.c.
nstiscsi_get_initiator_oid explicitly returns -1 on failure and the
comments in the return value should say this. Currently it says !=0
which could lead one to expect multiple return values.
-Sanjay
-Sanjay
> 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
>>
