Randy Dunlap wrote:
> Boaz Harrosh wrote:
>> Thank you Randy for your review, I will post a fixed
>> patch shortly. I have changed according to your comments
>> except in one place, see arguments below.
>>
>>>> ---
>>>> include/scsi/osd_initiator.h | 332 ++++++++++++++++++++++++++++
>>>> include/scsi/osd_protocol.h | 497 ++++++++++++++++++++++++++++++++++++++++++
>>>> include/scsi/osd_sec.h | 45 ++++
>>>> include/scsi/osd_types.h | 40 ++++
>>>> 4 files changed, 914 insertions(+), 0 deletions(-)
>>>> create mode 100644 include/scsi/osd_initiator.h
>>>> create mode 100644 include/scsi/osd_protocol.h
>>>> create mode 100644 include/scsi/osd_sec.h
>>>> create mode 100644 include/scsi/osd_types.h
>
>>>> +/**
>>> Don't start comment blocks with /** when they are not kernel-doc,
>>> like this one is not.
>>>
>> OK, I must confess my kernel-doc total ignorance. I was imagining that
>> each source file's kernel-doc comments are collected into an html file.
>> I thought that this comment will be like an introduction to the following
>> function-by-function reference. Anyway it's fixed
>
> You can choose to have comments included in the kernel-doc's collected
> output. You do this by using this notation:
>
> /** DOC <topic_name>:
> * <these lines are added to kernel-doc output when you use:
> !P<filename> <topic_name>
> * a Documentation/DocBook/*.tmpl file.
> */
>
> See Documentation/DocBook/mac80211.tmpl for examples.
>
> Johannes, I thought that you had some usage documentation for DOC:.
> Did you not or did it not get merged??
> It needs to be added to Documentation/kernel-doc-nano-HOWTO.txt.
>
OK Thanks I'll give it a shot
<snip>
>>>> +/**
>>>> + * osd_execute_request - Execute the request synchronously through
>>>> + * the block-layer
>>> Function name and short description need to be on one line.
>>>
>> OK I re-worded so it will fit in one line. What happens if it does not
>> fit, both name and description, in 80 characters? is there a continuation
>> symbol or something?
>
> Nope. It can (a) be longer than 80 characters (an exception is made here)
> or (b) split up like this:
>
> /**
> * func_name - some short description here
> * @prm1: prm1 description
> * @prmn: prmn description
> *
> * <longer function description here>
> */
>
OK So I guess I took (b). Thanks.
<snip>
>>>> +/* (osd-r10:4.9.2.2)
>>>> + * osd2r03:4.11.2.2 Capability format
>>>> + */
>>>> +struct osd_capability_head {
>>>> + u8 format; /* low nibble */
>>>> + u8 integrity_algorithm__key_version; /* MAKE_BYTE(integ_alg, key_ver) */
>>>> + u8 security_method;
>>>> + u8 reserved1;
>>>> +/*04*/ struct osd_timestamp expiration_time;
>>>> +/*10*/ u8 audit[30-10];
>>>> +/*30*/ u8 discriminator[42-30];
>>>> +/*42*/ struct osd_timestamp object_created_time;
>>>> +/*48*/ u8 object_type;
>>>> + u8 permissions_bit_mask[54-49];
>>> The offset comments are OK with me, but please lose the [b-a] length specifiers.
>>>
>> I would, please, like to keep them. For the user it does not matter.
>> Because he is not suppose to care if he is doing:
>> - memset(och->permissions_bit_mask, 0, 5); // BAD
>> + memset(och->permissions_bit_mask, 0, sizeof(och->permissions_bit_mask)); // GOOD
>>
>> But for the protocol reader / debuggerer this is much easier since this is the
>> way he will see them on the wire and the way it is laid out in the standard text.
>>
>> It was much easier to read the standard text and develop the header this way, complicated
>> by the fact that OSD v2 was a moving target and the changes from OSD v1. And it helped in
>> finding bugs. Now to go over all of them and calculate the difference and remove it. I'm
>> loosing information, and I feel sad to loose it.
>>
>> But if you are totally not convinced I will remove them?
>
> I've debugged plenty of code so I'll respectfully disagree with you.
> It's confusing and ugly. But I don't control whether it is merged upstream
> or not.
>
If it's "confusing and ugly" then that's bad. I guess I'm so much into the
standard-text, that I like it this way, but not so for an onlooker.
I'll change it. Last thing I want is ugly confusing code.
Thanks again
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
