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.
>>> + * How to use the osd library:
>>> + *
>>> + * osd_start_request
>>> + * Allocates a request.
>>> + *
>>> + * osd_req_*
>>> + * Call one of, to encode the desired operation.
>>> + *
>>> + * osd_add_{get,set}_attr
>>> + * Optionally add attributes to the CDB, list or page mode.
>>> + *
>>> + * osd_finalize_request
>>> + * Computes final data out/in offsets and signs the request,
>>> + * making it ready for execution.
>>> + *
>>> + * osd_execute_request
>>> + * May be called to execute it through the block layer. Other wise submit
>>> + * the associated block request in some other way.
>>> + *
>>> + * After execution:
>>> + * osd_req_decode_sense
>>> + * Decodes sense information to verify execution results.
>>> + *
>>> + * osd_req_decode_get_attr
>>> + * Retrieve osd_add_get_attr_list() values if used.
>>> + *
>>> + * osd_end_request
>>> + * Must be called to deallocate the request.
>>> + */
>>> +int osd_finalize_request(struct osd_request *or,
>>> + u8 options, const void *cap, const u8 *cap_key);
>>> +
>>> +/**
>>> + * 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>
*/
>>> + * @or: osd_request to Executed
>>> + *
>>> + * Calls blk_execute_rq to q the command and waits for completion.
>>> + */
>>> +int osd_execute_request(struct osd_request *or);
>>> diff --git a/include/scsi/osd_protocol.h b/include/scsi/osd_protocol.h
>>> new file mode 100644
>>> index 0000000..77a74a3
>>> --- /dev/null
>>> +++ b/include/scsi/osd_protocol.h
>>> @@ -0,0 +1,497 @@
>>> +/*
>>> + * osd_protocol.h - OSD T10 standard C definitions.
>>> + *
>>> + * Copyright (C) 2008 Panasas Inc. All rights reserved.
>>> + *
>>> + * Authors:
>>> + * Boaz Harrosh <bharrosh@xxxxxxxxxxx>
>>> + * Benny Halevy <bhalevy@xxxxxxxxxxx>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2
>>> + *
>>> + * This file contains types and constants that are defined by the protocol
>>> + * Note: All names and symbols are taken from the OSD standard's text.
>>> + */
>>> +#ifndef __OSD_PROTOCOL_H__
>>> +#define __OSD_PROTOCOL_H__
>>> +
>>> +#include <linux/types.h>
>>> +#include <asm/unaligned.h>
>>> +#include <scsi/scsi.h>
...
>>> +/* (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.
>>> +/*54*/ u8 reserved2;
>>> +/*55*/ u8 object_descriptor_type; /* high nibble */
>>> +} __packed;
--
~Randy
--
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
