[PATCH 0/3] isdn: patches for 2.6.31

[PATCH 0/3] isdn: patches for 2.6.31

[Tilman Schmidt]

Dave, Karsten,

I would like to propose the following three patches to the Kernel CAPI
subsystem for inclusion in kernel release 2.6.31.
The first one was proposed four weeks ago and so far hasn't met any
protests.
The second one is a rather trivial bugfix and documentation improvement.
The third one just adds a few new insights about the workings of the
CAPI subsystem to the Documentation file, INTERFACE.CAPI.

Thanks,
Tilman

[PATCH 1/3] isdn: rename capi_ctr_reseted() to capi_ctr_down()

[Tilman Schmidt]

Change the name of the Kernel CAPI exported function capi_ctr_reseted()
to something representing its purpose better.

Impact: function renamed, no functional change
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 Documentation/isdn/INTERFACE.CAPI |    4 ++--
 drivers/isdn/capi/kcapi.c         |    8 ++++----
 drivers/isdn/hardware/avm/b1.c    |    2 +-
 drivers/isdn/hardware/avm/b1dma.c |    2 +-
 drivers/isdn/hardware/avm/c4.c    |    4 ++--
 drivers/isdn/hardware/avm/t1isa.c |    2 +-
 drivers/isdn/hysdn/hycapi.c       |    4 ++--
 include/linux/isdn/capilli.h      |    2 +-
 net/bluetooth/cmtp/capi.c         |    2 +-
 9 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/Documentation/isdn/INTERFACE.CAPI b/Documentation/isdn/INTERFACE.CAPI
index 786d619..4631538 100644
--- a/Documentation/isdn/INTERFACE.CAPI
+++ b/Documentation/isdn/INTERFACE.CAPI
@@ -45,7 +45,7 @@ From then on, Kernel CAPI may call the registered callback functions for the
 device.
 
 If the device becomes unusable for any reason (shutdown, disconnect ...), the
-driver has to call capi_ctr_reseted(). This will prevent further calls to the
+driver has to call capi_ctr_down(). This will prevent further calls to the
 callback functions by Kernel CAPI.
 
 
@@ -166,7 +166,7 @@ int detach_capi_ctr(struct capi_ctr *ctrlr)
 	register/unregister a device (controller) with Kernel CAPI
 
 void capi_ctr_ready(struct capi_ctr *ctrlr)
-void capi_ctr_reseted(struct capi_ctr *ctrlr)
+void capi_ctr_down(struct capi_ctr *ctrlr)
 	signal controller ready/not ready
 
 void capi_ctr_suspend_output(struct capi_ctr *ctrlr)
diff --git a/drivers/isdn/capi/kcapi.c b/drivers/isdn/capi/kcapi.c
index f331703..57d2636 100644
--- a/drivers/isdn/capi/kcapi.c
+++ b/drivers/isdn/capi/kcapi.c
@@ -377,14 +377,14 @@ void capi_ctr_ready(struct capi_ctr * card)
 EXPORT_SYMBOL(capi_ctr_ready);
 
 /**
- * capi_ctr_reseted() - signal CAPI controller reset
+ * capi_ctr_down() - signal CAPI controller not ready
  * @card:	controller descriptor structure.
  *
  * Called by hardware driver to signal that the controller is down and
  * unavailable for use.
  */
 
-void capi_ctr_reseted(struct capi_ctr * card)
+void capi_ctr_down(struct capi_ctr * card)
 {
 	u16 appl;
 
@@ -413,7 +413,7 @@ void capi_ctr_reseted(struct capi_ctr * card)
 	notify_push(KCI_CONTRDOWN, card->cnr, 0, 0);
 }
 
-EXPORT_SYMBOL(capi_ctr_reseted);
+EXPORT_SYMBOL(capi_ctr_down);
 
 /**
  * capi_ctr_suspend_output() - suspend controller
@@ -517,7 +517,7 @@ EXPORT_SYMBOL(attach_capi_ctr);
 int detach_capi_ctr(struct capi_ctr *card)
 {
         if (card->cardstate != CARD_DETECTED)
-		capi_ctr_reseted(card);
+		capi_ctr_down(card);
 
 	ncards--;
 
diff --git a/drivers/isdn/hardware/avm/b1.c b/drivers/isdn/hardware/avm/b1.c
index abf05ec..a7c0083 100644
--- a/drivers/isdn/hardware/avm/b1.c
+++ b/drivers/isdn/hardware/avm/b1.c
@@ -330,7 +330,7 @@ void b1_reset_ctr(struct capi_ctr *ctrl)
 	spin_lock_irqsave(&card->lock, flags);
 	capilib_release(&cinfo->ncci_head);
 	spin_unlock_irqrestore(&card->lock, flags);
-	capi_ctr_reseted(ctrl);
+	capi_ctr_down(ctrl);
 }
 
 void b1_register_appl(struct capi_ctr *ctrl,
diff --git a/drivers/isdn/hardware/avm/b1dma.c b/drivers/isdn/hardware/avm/b1dma.c
index da34b98..0e84aaa 100644
--- a/drivers/isdn/hardware/avm/b1dma.c
+++ b/drivers/isdn/hardware/avm/b1dma.c
@@ -759,7 +759,7 @@ void b1dma_reset_ctr(struct capi_ctr *ctrl)
 	memset(cinfo->version, 0, sizeof(cinfo->version));
 	capilib_release(&cinfo->ncci_head);
 	spin_unlock_irqrestore(&card->lock, flags);
-	capi_ctr_reseted(ctrl);
+	capi_ctr_down(ctrl);
 }
 
 /* ------------------------------------------------------------- */
diff --git a/drivers/isdn/hardware/avm/c4.c b/drivers/isdn/hardware/avm/c4.c
index 9df1d3f..6833301 100644
--- a/drivers/isdn/hardware/avm/c4.c
+++ b/drivers/isdn/hardware/avm/c4.c
@@ -681,7 +681,7 @@ static irqreturn_t c4_handle_interrupt(avmcard *card)
 			spin_lock_irqsave(&card->lock, flags);
 			capilib_release(&cinfo->ncci_head);
 			spin_unlock_irqrestore(&card->lock, flags);
-			capi_ctr_reseted(&cinfo->capi_ctrl);
+			capi_ctr_down(&cinfo->capi_ctrl);
 		}
 		card->nlogcontr = 0;
 		return IRQ_HANDLED;
@@ -909,7 +909,7 @@ static void c4_reset_ctr(struct capi_ctr *ctrl)
         for (i=0; i < card->nr_controllers; i++) {
 		cinfo = &card->ctrlinfo[i];
 		memset(cinfo->version, 0, sizeof(cinfo->version));
-		capi_ctr_reseted(&cinfo->capi_ctrl);
+		capi_ctr_down(&cinfo->capi_ctrl);
 	}
 	card->nlogcontr = 0;
 }
diff --git a/drivers/isdn/hardware/avm/t1isa.c b/drivers/isdn/hardware/avm/t1isa.c
index e772449..1c53fd4 100644
--- a/drivers/isdn/hardware/avm/t1isa.c
+++ b/drivers/isdn/hardware/avm/t1isa.c
@@ -339,7 +339,7 @@ static void t1isa_reset_ctr(struct capi_ctr *ctrl)
 	spin_lock_irqsave(&card->lock, flags);
 	capilib_release(&cinfo->ncci_head);
 	spin_unlock_irqrestore(&card->lock, flags);
-	capi_ctr_reseted(ctrl);
+	capi_ctr_down(ctrl);
 }
 
 static void t1isa_remove(struct pci_dev *pdev)
diff --git a/drivers/isdn/hysdn/hycapi.c b/drivers/isdn/hysdn/hycapi.c
index 53f6ad1..4ffaa14 100644
--- a/drivers/isdn/hysdn/hycapi.c
+++ b/drivers/isdn/hysdn/hycapi.c
@@ -67,7 +67,7 @@ hycapi_reset_ctr(struct capi_ctr *ctrl)
 	printk(KERN_NOTICE "HYCAPI hycapi_reset_ctr\n");
 #endif
 	capilib_release(&cinfo->ncci_head);
-	capi_ctr_reseted(ctrl);
+	capi_ctr_down(ctrl);
 }
 
 /******************************
@@ -347,7 +347,7 @@ int hycapi_capi_stop(hysdn_card *card)
 	if(cinfo) {
 		ctrl = &cinfo->capi_ctrl;
 /*		ctrl->suspend_output(ctrl); */
-		capi_ctr_reseted(ctrl);
+		capi_ctr_down(ctrl);
 	}
 	return 0;
 }
diff --git a/include/linux/isdn/capilli.h b/include/linux/isdn/capilli.h
index 35e9b0f..7acb87a 100644
--- a/include/linux/isdn/capilli.h
+++ b/include/linux/isdn/capilli.h
@@ -79,7 +79,7 @@ int attach_capi_ctr(struct capi_ctr *);
 int detach_capi_ctr(struct capi_ctr *);
 
 void capi_ctr_ready(struct capi_ctr * card);
-void capi_ctr_reseted(struct capi_ctr * card);
+void capi_ctr_down(struct capi_ctr * card);
 void capi_ctr_suspend_output(struct capi_ctr * card);
 void capi_ctr_resume_output(struct capi_ctr * card);
 void capi_ctr_handle_message(struct capi_ctr * card, u16 appl, struct sk_buff *skb);
diff --git a/net/bluetooth/cmtp/capi.c b/net/bluetooth/cmtp/capi.c
index 78958c0..97f8d68 100644
--- a/net/bluetooth/cmtp/capi.c
+++ b/net/bluetooth/cmtp/capi.c
@@ -382,7 +382,7 @@ static void cmtp_reset_ctr(struct capi_ctr *ctrl)
 
 	BT_DBG("ctrl %p", ctrl);
 
-	capi_ctr_reseted(ctrl);
+	capi_ctr_down(ctrl);
 
 	atomic_inc(&session->terminate);
 	cmtp_schedule(session);
-- 
1.6.2.1.214.ge986c

Re: [PATCH 1/3] isdn: rename capi_ctr_reseted() to capi_ctr_down()

[Marcel Holtmann]

Hi Tilman,

> Change the name of the Kernel CAPI exported function capi_ctr_reseted()
> to something representing its purpose better.
> 
> Impact: function renamed, no functional change
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> ---
>  Documentation/isdn/INTERFACE.CAPI |    4 ++--
>  drivers/isdn/capi/kcapi.c         |    8 ++++----
>  drivers/isdn/hardware/avm/b1.c    |    2 +-
>  drivers/isdn/hardware/avm/b1dma.c |    2 +-
>  drivers/isdn/hardware/avm/c4.c    |    4 ++--
>  drivers/isdn/hardware/avm/t1isa.c |    2 +-
>  drivers/isdn/hysdn/hycapi.c       |    4 ++--
>  include/linux/isdn/capilli.h      |    2 +-
>  net/bluetooth/cmtp/capi.c         |    2 +-
>  9 files changed, 15 insertions(+), 15 deletions(-)

for the Bluetooth part:

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

[PATCH 2/3] isdn: prevent NULL ptr oops in capi_cmsg2str()

[Tilman Schmidt]

The capi_cmsg2str() function has the undocumented requirement that one
of the functions capi_cmsg2message() or capi_message2cmsg() must have
been called before it, otherwise a NULL pointer dereference occurs.
This patch adds a NULL pointer check to avoid the Oops, and also adds
kerneldoc comments to the exported functions in capiutil.c.

Impact: documentation and error handling improvement, no functional change
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 drivers/isdn/capi/capiutil.c |   67 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/capi/capiutil.c b/drivers/isdn/capi/capiutil.c
index 29419a8..35ba711 100644
--- a/drivers/isdn/capi/capiutil.c
+++ b/drivers/isdn/capi/capiutil.c
@@ -490,7 +490,14 @@ static void pars_2_message(_cmsg * cmsg)
 	}
 }
 
-/*-------------------------------------------------------*/
+/**
+ * capi_cmsg2message() - assemble CAPI 2.0 message from _cmsg structure
+ * @cmsg:	_cmsg structure
+ * @msg:	buffer for assembled message
+ *
+ * Return value: 0 for success
+ */
+
 unsigned capi_cmsg2message(_cmsg * cmsg, u8 * msg)
 {
 	cmsg->m = msg;
@@ -553,7 +560,14 @@ static void message_2_pars(_cmsg * cmsg)
 	}
 }
 
-/*-------------------------------------------------------*/
+/**
+ * capi_message2cmsg() - disassemble CAPI 2.0 message into _cmsg structure
+ * @cmsg:	_cmsg structure
+ * @msg:	buffer for assembled message
+ *
+ * Return value: 0 for success
+ */
+
 unsigned capi_message2cmsg(_cmsg * cmsg, u8 * msg)
 {
 	memset(cmsg, 0, sizeof(_cmsg));
@@ -573,7 +587,18 @@ unsigned capi_message2cmsg(_cmsg * cmsg, u8 * msg)
 	return 0;
 }
 
-/*-------------------------------------------------------*/
+/**
+ * capi_cmsg_header() - initialize header part of _cmsg structure
+ * @cmsg:	_cmsg structure
+ * @_ApplId:	ApplID field value
+ * @_Command:	Command field value
+ * @_Subcommand:	Subcommand field value
+ * @_Messagenumber:	Message Number field value
+ * @_Controller:	Controller/PLCI/NCCI field value
+ *
+ * Return value: 0 for success
+ */
+
 unsigned capi_cmsg_header(_cmsg * cmsg, u16 _ApplId,
 			  u8 _Command, u8 _Subcommand,
 			  u16 _Messagenumber, u32 _Controller)
@@ -641,6 +666,14 @@ static char *mnames[] =
 	[0x4e] = "MANUFACTURER_RESP"
 };
 
+/**
+ * capi_cmd2str() - convert CAPI 2.0 command/subcommand number to name
+ * @cmd:	command number
+ * @subcmd:	subcommand number
+ *
+ * Return value: static string
+ */
+
 char *capi_cmd2str(u8 cmd, u8 subcmd)
 {
 	return mnames[command_2_index(cmd, subcmd)];
@@ -879,6 +912,11 @@ init:
 	return cdb;
 }
 
+/**
+ * cdebbuf_free() - free CAPI debug buffer
+ * @cdb:	buffer to free
+ */
+
 void cdebbuf_free(_cdebbuf *cdb)
 {
 	if (likely(cdb == g_debbuf)) {
@@ -891,6 +929,16 @@ void cdebbuf_free(_cdebbuf *cdb)
 }
 
 
+/**
+ * capi_message2str() - format CAPI 2.0 message for printing
+ * @msg:	CAPI 2.0 message
+ *
+ * Allocates a CAPI debug buffer and fills it with a printable representation
+ * of the CAPI 2.0 message in @msg.
+ * Return value: allocated debug buffer, NULL on error
+ * The returned buffer should be freed by a call to cdebbuf_free() after use.
+ */
+
 _cdebbuf *capi_message2str(u8 * msg)
 {
 	_cdebbuf *cdb;
@@ -926,10 +974,23 @@ _cdebbuf *capi_message2str(u8 * msg)
 	return cdb;
 }
 
+/**
+ * capi_cmsg2str() - format _cmsg structure for printing
+ * @cmsg:	_cmsg structure
+ *
+ * Allocates a CAPI debug buffer and fills it with a printable representation
+ * of the CAPI 2.0 message stored in @cmsg by a previous call to
+ * capi_cmsg2message() or capi_message2cmsg().
+ * Return value: allocated debug buffer, NULL on error
+ * The returned buffer should be freed by a call to cdebbuf_free() after use.
+ */
+
 _cdebbuf *capi_cmsg2str(_cmsg * cmsg)
 {
 	_cdebbuf *cdb;
 
+	if (!cmsg->m)
+		return NULL;	/* no message */
 	cdb = cdebbuf_alloc();
 	if (!cdb)
 		return NULL;
-- 
1.6.2.1.214.ge986c

[PATCH 3/3] isdn: extend INTERFACE.CAPI document

[Tilman Schmidt]

Add description of the _cmsg structure and helper functions,
information about concurrency of the callback methods, and a
description of the return value of the send_message method.

Impact: documentation
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
---
 Documentation/isdn/INTERFACE.CAPI |   84 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/Documentation/isdn/INTERFACE.CAPI b/Documentation/isdn/INTERFACE.CAPI
index 4631538..5c5a954 100644
--- a/Documentation/isdn/INTERFACE.CAPI
+++ b/Documentation/isdn/INTERFACE.CAPI
@@ -114,20 +114,36 @@ char *driver_name
 int (*load_firmware)(struct capi_ctr *ctrlr, capiloaddata *ldata)
 	(optional) pointer to a callback function for sending firmware and
 	configuration data to the device
+	Return value: 0 on success, error code on error
+	Called in process context.
 
 void (*reset_ctr)(struct capi_ctr *ctrlr)
-	pointer to a callback function for performing a reset on the device,
-	releasing all registered applications
+	(optional) pointer to a callback function for performing a reset on
+	the device, releasing all registered applications
+	Called in process context.
 
 void (*register_appl)(struct capi_ctr *ctrlr, u16 applid,
 			capi_register_params *rparam)
 void (*release_appl)(struct capi_ctr *ctrlr, u16 applid)
 	pointers to callback functions for registration and deregistration of
 	applications with the device
+	Calls to these functions are serialized by Kernel CAPI so that only
+	one call to any of them is active at any time.
 
 u16  (*send_message)(struct capi_ctr *ctrlr, struct sk_buff *skb)
 	pointer to a callback function for sending a CAPI message to the
 	device
+	Return value: CAPI error code
+	If the method returns 0 (CAPI_NOERROR) the driver has taken ownership
+	of the skb and the caller may no longer access it. If it returns a
+	non-zero (error) value then ownership of the skb returns to the caller
+	who may reuse or free it.
+	The return value should only be used to signal problems with respect
+	to accepting or queueing the message. Errors occurring during the
+	actual processing of the message should be signaled with an
+	appropriate reply message.
+	Calls to this function are not serialized by Kernel CAPI, ie. it must
+	be prepared to be re-entered.
 
 char *(*procinfo)(struct capi_ctr *ctrlr)
 	pointer to a callback function returning the entry for the device in
@@ -138,6 +154,8 @@ read_proc_t *ctr_read_proc
 	system entry, /proc/capi/controllers/<n>; will be called with a
 	pointer to the device's capi_ctr structure as the last (data) argument
 
+Note: Callback functions are never called in interrupt context.
+
 - to be filled in before calling capi_ctr_ready():
 
 u8 manu[CAPI_MANUFACTURER_LEN]
@@ -153,6 +171,45 @@ u8 serial[CAPI_SERIAL_LEN]
 	value to return for CAPI_GET_SERIAL
 
 
+4.3 The _cmsg Structure
+
+(declared in <linux/isdn/capiutil.h>)
+
+The _cmsg structure stores the contents of a CAPI 2.0 message in an easily
+accessible form. It contains members for all possible CAPI 2.0 parameters, of
+which only those appearing in the message type currently being processed are
+actually used. Unused members should be set to zero.
+
+Members are named after the CAPI 2.0 standard names of the parameters they
+represent. See <linux/isdn/capiutil.h> for the exact spelling. Member data
+types are:
+
+u8          for CAPI parameters of type 'byte'
+
+u16         for CAPI parameters of type 'word'
+
+u32         for CAPI parameters of type 'dword'
+
+_cstruct    for CAPI parameters of type 'struct' not containing any
+	    variably-sized (struct) subparameters (eg. 'Called Party Number')
+	    The member is a pointer to a buffer containing the parameter in
+	    CAPI encoding (length + content). It may also be NULL, which will
+	    be taken to represent an empty (zero length) parameter.
+
+_cmstruct   for CAPI parameters of type 'struct' containing 'struct'
+	    subparameters ('Additional Info' and 'B Protocol')
+	    The representation is a single byte containing one of the values:
+	    CAPI_DEFAULT: the parameter is empty
+	    CAPI_COMPOSE: the values of the subparameters are stored
+	    individually in the corresponding _cmsg structure members
+
+Functions capi_cmsg2message() and capi_message2cmsg() are provided to convert
+messages between their transport encoding described in the CAPI 2.0 standard
+and their _cmsg structure representation. Note that capi_cmsg2message() does
+not know or check the size of its destination buffer. The caller must make
+sure it is big enough to accomodate the resulting CAPI message.
+
+
 5. Lower Layer Interface Functions
 
 (declared in <linux/isdn/capilli.h>)
@@ -211,3 +268,26 @@ CAPIMSG_CONTROL(m)	CAPIMSG_SETCONTROL(m, contr)	Controller/PLCI/NCCI
 							(u32)
 CAPIMSG_DATALEN(m)	CAPIMSG_SETDATALEN(m, len)	Data Length (u16)
 
+
+Library functions for working with _cmsg structures
+(from <linux/isdn/capiutil.h>):
+
+unsigned capi_cmsg2message(_cmsg *cmsg, u8 *msg)
+	Assembles a CAPI 2.0 message from the parameters in *cmsg, storing the
+	result in *msg.
+
+unsigned capi_message2cmsg(_cmsg *cmsg, u8 *msg)
+	Disassembles the CAPI 2.0 message in *msg, storing the parameters in
+	*cmsg.
+
+unsigned capi_cmsg_header(_cmsg *cmsg, u16 ApplId, u8 Command, u8 Subcommand,
+			  u16 Messagenumber, u32 Controller)
+	Fills the header part and address field of the _cmsg structure *cmsg
+	with the given values, zeroing the remainder of the structure so only
+	parameters with non-default values need to be changed before sending
+	the message.
+
+void capi_cmsg_answer(_cmsg *cmsg)
+	Sets the low bit of the Subcommand field in *cmsg, thereby converting
+	_REQ to _CONF and _IND to _RESP.
+
-- 
1.6.2.1.214.ge986c

Re: [PATCH 0/3] isdn: patches for 2.6.31

[David Miller]

From: Tilman Schmidt <tilman@imap.cc>
Date: Sun, 31 May 2009 01:32:04 +0200 (CEST)

> I would like to propose the following three patches to the Kernel CAPI
> subsystem for inclusion in kernel release 2.6.31.
> The first one was proposed four weeks ago and so far hasn't met any
> protests.
> The second one is a rather trivial bugfix and documentation improvement.
> The third one just adds a few new insights about the workings of the
> CAPI subsystem to the Documentation file, INTERFACE.CAPI.

I'm not applying this patch series.

Especially I dislike the second patch.

First problem in the second patch.  You're doing two things
at once.  You're adding function documentation and also adding
a NULL pointer check.

Second problem, the NULL pointer check is gratuitous.  Document
that the 'm' member has to be non-NULL and leave the check out.

Thanks.

Re: [PATCH 0/3] isdn: patches for 2.6.31

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

On 01.06.2009 12:04, David Miller wrote:
> First problem in the second patch.  You're doing two things
> at once.  You're adding function documentation and also adding
> a NULL pointer check.

No problem. I can split the patch in two if you prefer it that way.

> Second problem, the NULL pointer check is gratuitous.  Document
> that the 'm' member has to be non-NULL and leave the check out.

That would be a bad solution for two reasons:
First, the 'm' member is private to capiutil.{c,h}. Callers are
not supposed to access it. Therefore it shouldn't be referred to
in the interface documentation. At best, such a mention would
leave users of the function confused how to assure that condition.
At worst, it might mislead them into meddling directly with the
member, thereby producing incorrect code.
And second, the main use of capi_cmsg2str() is for error reporting
and debugging output. Oopsing in an error handler is particularly
troublesome. At the same time, the risk of the 'm' member being
unexpectedly NULL is particularly high when something has gone
wrong already. So a safety check is advisable in this case.

Thanks,
Tilman

PS: Any objections against the other two patches?

-- 
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [PATCH 0/3] isdn: patches for 2.6.31

[David Miller]

From: Tilman Schmidt <tilman@imap.cc>
Date: Sun, 07 Jun 2009 00:38:26 +0200

> On 01.06.2009 12:04, David Miller wrote:
>> First problem in the second patch.  You're doing two things
>> at once.  You're adding function documentation and also adding
>> a NULL pointer check.
> 
> No problem. I can split the patch in two if you prefer it that way.

Thanks.

>> Second problem, the NULL pointer check is gratuitous.  Document
>> that the 'm' member has to be non-NULL and leave the check out.
> 
> That would be a bad solution for two reasons:
> First, the 'm' member is private to capiutil.{c,h}. Callers are
> not supposed to access it. Therefore it shouldn't be referred to
> in the interface documentation. At best, such a mention would
> leave users of the function confused how to assure that condition.
> At worst, it might mislead them into meddling directly with the
> member, thereby producing incorrect code.
> And second, the main use of capi_cmsg2str() is for error reporting
> and debugging output. Oopsing in an error handler is particularly
> troublesome. At the same time, the risk of the 'm' member being
> unexpectedly NULL is particularly high when something has gone
> wrong already. So a safety check is advisable in this case.

Fair enough.

> PS: Any objections against the other two patches?

I don't remember it's been so many days since I looked at
this series :-(