PATCH] [ConfigFS]: Extend CONFIGFS_ATTR_STRUCT() and CONFIGFS_ATTR_OPS() macros

PATCH] [ConfigFS]: Extend CONFIGFS_ATTR_STRUCT() and CONFIGFS_ATTR_OPS() macros

[Nicholas A. Bellinger]

Hi Joel,

What do you think..?

Also, here is the drivers/lio-core/iscsi_target_configfs.c commit to
convert to use to new parameters for CONFIGFS_ATTR_STRUCT() and
CONFIGFS_ATTR_OPS():

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=e305460ebaa92ce1f8542a55bd62648904fb2fd6

I don't see anyone except for drivers/lio-core and
Documentation/filesystems/configfs/ using these macros..  If I can get
an ACK I would be happy to update the examples.

--nab


>From 71fc659e337dcc5941f6f64f6c8abfb6b477fd7c Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Wed, 22 Oct 2008 14:49:01 -0700
Subject: [PATCH] [ConfigFS]: Extend CONFIGFS_ATTR_STRUCT() and CONFIGFS_ATTR_OPS() macros

Extend CONFIGFS_ATTR_STRUCT() and CONFIGFS_ATTR_OPS() macros to allow
multiple struct config_item or struct config_group to hang off the same
structure.  This means the previous '_item' arguement gets split into
'_name' (for function and attribute structure names) and '_item' (the
struct _item used throughout the macros that contains
struct _item->my_config_group.

This patch also removed the requirement for code using these macros to
provide their own 'to_<name>()' function, which is now defined by
CONFIGFS_ATTR_OPS() by passing the '_item_member' argument which is
used inside of to_##_name() for locating struct _item *_item = container_of().

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 include/linux/configfs.h |   51 ++++++++++++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 7f62777..2eab7ef 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -132,11 +132,13 @@ struct configfs_attribute {
  * attributes, containing a configfs_attribute member and function pointers
  * for the show() and store() operations on that attribute. If they don't
  * need anything else on the extended attribute structure, they can use
- * this macro to define it  The argument _item is the name of the
- * config_item structure.
+ * this macro to define it.  The argument _name isends up as
+ * 'struct _name_attribute, as well as names of to CONFIGFS_ATTR_OPS() below.
+ * The argument _item is the name of the structure containing the
+ * struct config_item or struct config_group structure members
  */
-#define CONFIGFS_ATTR_STRUCT(_item)					\
-struct _item##_attribute {						\
+#define CONFIGFS_ATTR_STRUCT(_name, _item)				\
+struct _name##_attribute {						\
 	struct configfs_attribute attr;					\
 	ssize_t (*show)(struct _item *, char *);			\
 	ssize_t (*store)(struct _item *, const char *, size_t);		\
@@ -175,35 +177,44 @@ struct _item##_attribute {						\
  * With these extended attributes, the simple show_attribute() and
  * store_attribute() operations need to call the show() and store() of the
  * attributes.  This is a common pattern, so we provide a macro to define
- * them.  The argument _item is the name of the config_item structure.
- * This macro expects the attributes to be named "struct <name>_attribute"
- * and the function to_<name>() to exist;
+ * them.  The argument _name is the name of the attribute defined by
+ * CONFIGFS_ATTR_STRUCT(). The argument _item is the name of the structure
+ * containing the struct config_item or struct config_group structure member.
+ * The argument _item_member is the actual name of the struct config_* struct
+ * in your _item structure.  Meaning  my_structure->some_config_group.
+ *		                      ^^_item^^^^^  ^^_item_member^^^
+ * This macro expects the attributes to be named "struct <name>_attribute".
  */
-#define CONFIGFS_ATTR_OPS(_item)					\
-static ssize_t _item##_attr_show(struct config_item *item,		\
+#define CONFIGFS_ATTR_OPS(_name, _item, _item_member)			\
+static struct _item *to_##_name(struct config_item *ci)			\
+{									\
+	return((ci) ? container_of(to_config_group(ci), struct _item,	\
+		_item_member) : NULL);					\
+}									\
+static ssize_t _name##_attr_show(struct config_item *item,		\
 				 struct configfs_attribute *attr,	\
 				 char *page)				\
 {									\
-	struct _item *_item = to_##_item(item);				\
-	struct _item##_attribute *_item##_attr =			\
-		container_of(attr, struct _item##_attribute, attr);	\
+	struct _item *_item = to_##_name(item);				\
+	struct _name##_attribute *_name##_attr =			\
+		container_of(attr, struct _name##_attribute, attr);	\
 	ssize_t ret = 0;						\
 									\
-	if (_item##_attr->show)						\
-		ret = _item##_attr->show(_item, page);			\
+	if (_name##_attr->show)						\
+		ret = _name##_attr->show(_item, page);			\
 	return ret;							\
 }									\
-static ssize_t _item##_attr_store(struct config_item *item,		\
+static ssize_t _name##_attr_store(struct config_item *item,		\
 				  struct configfs_attribute *attr,	\
 				  const char *page, size_t count)	\
 {									\
-	struct _item *_item = to_##_item(item);				\
-	struct _item##_attribute *_item##_attr =			\
-		container_of(attr, struct _item##_attribute, attr);	\
+	struct _item *_item = to_##_name(item);				\
+	struct _name##_attribute *_name##_attr =			\
+		container_of(attr, struct _name##_attribute, attr);	\
 	ssize_t ret = -EINVAL;						\
 									\
-	if (_item##_attr->store)					\
-		ret = _item##_attr->store(_item, page, count);		\
+	if (_name##_attr->store)					\
+		ret = _name##_attr->store(_item, page, count);		\
 	return ret;							\
 }
 
-- 
1.5.4.1

Re: PATCH] [ConfigFS]: Extend CONFIGFS_ATTR_STRUCT() and CONFIGFS_ATTR_OPS() macros

[Joel Becker]

On Wed, Oct 22, 2008 at 03:06:29PM -0700, Nicholas A. Bellinger wrote:
> Hi Joel,
> 
> What do you think..?
> 
> Also, here is the drivers/lio-core/iscsi_target_configfs.c commit to
> convert to use to new parameters for CONFIGFS_ATTR_STRUCT() and
> CONFIGFS_ATTR_OPS():

	It seems even more complex, and inconsistent with what sysfs
and other users do.  Can you give me an example of using it?

Joel

-- 

"Not everything that can be counted counts, and not everything
 that counts can be counted."
        - Albert Einstein 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: PATCH] [ConfigFS]: Extend CONFIGFS_ATTR_STRUCT() and CONFIGFS_ATTR_OPS() macros

[Nicholas A. Bellinger]

On Wed, 2008-10-22 at 15:13 -0700, Joel Becker wrote:
> On Wed, Oct 22, 2008 at 03:06:29PM -0700, Nicholas A. Bellinger wrote:
> > Hi Joel,
> > 
> > What do you think..?
> > 
> > Also, here is the drivers/lio-core/iscsi_target_configfs.c commit to
> > convert to use to new parameters for CONFIGFS_ATTR_STRUCT() and
> > CONFIGFS_ATTR_OPS():
> 
> 	It seems even more complex, and inconsistent with what sysfs
> and other users do.  Can you give me an example of using it?
> 

Sure, my reasoning was as I was looking at converting as much code as
possible in target_core_config.c and iscsi_target_configfs.c to use the
include/linux/config.h wrappers, I have noticed I am running into a
limitiation with existing code:

Having the user's attribute set of macro's '_name' being synonymous with
both the user's struct _item * and show()/store() defines with
CONFIGFS_ATTR_OPS() was going to cause namespace pollution when you want
to be able generic show()/store() on multiple struct config_groups
hanging off the same original struct _item.

For me, this was looking like problems when you have multiple default
struct config_groups off a single struct config_group,
namely /sys/kernel/config/target/iscsi/$IQN/$TPGT amoung other places in
LIO-Target.

Also, I would argue the patch makes the existing attribute macro in
configfs.h slightly *LESS* complex for the users of the macro because
you no longer have to define your own internal "to_<name>()" for each
struct config_group you want to hang attributes off.

Thanks!

--nab

> Joel
>