Re: [LTP] block: how shall register_blkdev() work?

["Németh Márton" <nm127@freemail.hu>]

Greg KH wrote:
> On Tue, Jan 13, 2009 at 07:15:49AM +0100, Németh Márton wrote:
>> Hi,
>> Greg KH wrote:
>>> On Mon, Jan 12, 2009 at 01:09:00PM +0530, Subrata Modak wrote:
>>>> Greh,
>>>>
>>>> Can you please help us in reviewing the following definition.
>>> Hm, why me?  I didn't write this code...
>> I couldn't find any note about the original author in the block/genhd.c and
>> I haven't received any answer from Jens Axboe, yet, who is mentioned in
>> the MAINTAINERS file as the maintainer of the Block layer. Maybe
>> you know somebody I should ask?
> 
> Al Viro?  LKML?

I added to the To/Cc list.
Thank you for your time to giving me valuable comments on this topic.

>>>>> +/**
>>>>> + * register_blkdev - register a new block device
>>>>> + *
>>>>> + * @major: [in] the requested major device number [1..255]. If
>>>>> + *         @major=0, try to allocate any unused major number.
>>>>> + * @name: [in] the name of the new block device
>>> What's with the [in] marking?  Is that a new kerneldoc markup?
>> I wanted to express that these parameters are inputs for the register_blkdev()
>> function.
> 
> But that is not used in any of the rest of the kernel, as they are
> easily surmised.  Don't add new markings that are not part of the
> standard please.

OK.

>>>>> + * ??? The @name must be unique within the system. ???
>>>>> + * OR: ??? You can use any zero terminated @name string. Note, however, if
>>>>> + *         you use a name which is already used it might mask the previous
>>>>> + *         device. ???
>>>>> + *
>>>>> + * Returns the allocated major number [1..255] or the return value is
>>>>> + * a negative error code if the wanted @major number is not available
>>>>> + * or there is no more major nubmers available when @major=0.
>>>>> + *
>>>>> + * ??? What does the return value 0 mean?
>>> Usually it means success, like almost all other api calls within the
>>> kernel.
>> Here is my new version:
>>
>> --- linux-2.6.28/block/genhd.c.orig	2008-12-25 00:26:37.000000000 +0100
>> +++ linux-2.6.28/block/genhd.c	2009-01-13 07:11:51.000000000 +0100
>> @@ -244,6 +244,25 @@ void blkdev_show(struct seq_file *seqf,
>>  }
>>  #endif /* CONFIG_PROC_FS */
>>
>> +/**
>> + * register_blkdev - register a new block device
>> + *
>> + * @major: [in] the requested major device number [1..255]. If
>> + *         @major=0, try to allocate any unused major number.
>> + * @name: [in] the name of the new block device
> 
> Drop the [in].
> 
> add, "as a zero terminated string" to the end of the @name description.
> 
>> + * ??? The @name must be unique within the system. ???
> 
> That's good enough.
> 
>> + * OR: ??? You can use any zero terminated @name string. Note, however, if
>> + *         you use a name which is already used it might mask the previous
>> + *         device.
> 
> "might"?  Don't you know from the code what it does if you pass a
> duplicate name in?

OK, this is not an exact documentation, rather a question. My goal is to find
out from the people what they *think* this function shall work and not how the
actual code works.

In case of testing it is essential what is the base of my test case. If I based
my test case on the source code I would test whether the compiler produces
good code or not. But my goal is different: to find out whether the function
works as the developers expect to work. If I try to find out this I can gain
that I can found possible differences between the code and the developer's
expectation.

My next version looks as follows. I still would like to find out what do
you *think* shall happen if the caller specifies a name which was already occupied
before:

--- linux-2.6.28/block/genhd.c.orig	2008-12-25 00:26:37.000000000 +0100
+++ linux-2.6.28/block/genhd.c	2009-01-13 07:37:37.000000000 +0100
@@ -244,6 +244,23 @@ void blkdev_show(struct seq_file *seqf,
 }
 #endif /* CONFIG_PROC_FS */

+/**
+ * register_blkdev - register a new block device
+ *
+ * @major: the requested major device number [1..255]. If @major=0, try to
+ *         allocate any unused major number.
+ * @name: the name of the new block device as a zero terminated string
+ *
+ * The @name must be unique within the system.
+ * ??? If you specify an already used name an error code will be returned. ???
+ *
+ * The return value depends on the @major input parameter.
+ *  - if a major device number was requested in range [1..255] then the
+ *    function returns zero on success, or a negative error code
+ *  - if any unused major number was requested with @major=0 parameter
+ *    then the return value is the allocated major number in range
+ *    [1..255] or a negative error code otherwise
+ */
 int register_blkdev(unsigned int major, const char *name)
 {
 	struct blk_major_name **n, *p;

Regards,

	Márton Németh