[PATCH 1/1] cgroups: introduce cft->read_seq()

[PATCH 1/1] cgroups: introduce cft->read_seq()

[Serge E. Hallyn]

Hi Paul,

the following (against 2.6.25-rc8-mm1) is a first attempt
at a simple seq_file usage in cgroups.  Comments much
appreciated.

thanks,
-serge

>From bd0977a5819dc43866fff325ae1e2726e747e2f4 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue@us.ibm.com>
Date: Wed, 2 Apr 2008 17:54:21 -0700
Subject: [PATCH 1/1] cgroups: introduce cft->read_seq()

Introduce a read_seq() helper in cftype, which uses
seq_file to print out lists.  Use it in the devices
cgroup.  Also split devices.allow into two files, so
now devices.deny and devices.allow are the ones to
use to manipulate the whitelist, while devices.list
outputs the cgroup's current whitelist.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 include/linux/cgroup.h   |    6 ++++
 kernel/cgroup.c          |   22 ++++++++++++-
 security/device_cgroup.c |   74 ++++++++++++++-------------------------------
 3 files changed, 49 insertions(+), 53 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 2d1d151..feb83dd 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -227,6 +227,12 @@ struct cftype {
 	 */
 	int (*read_map) (struct cgroup *cont, struct cftype *cft,
 			 struct cgroup_map_cb *cb);
+	/*
+	 * read_seq() is used for outputting a simple sequence
+	 * using seqfile.
+	 */
+	int (*read_seq) (struct cgroup *cont, struct cftype *cft,
+			 struct seq_file *m);
 
 	ssize_t (*write) (struct cgroup *cgrp, struct cftype *cft,
 			  struct file *file,
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 85f31ad..61dc509 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1518,7 +1518,7 @@ static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
 	return seq_printf(sf, "%s %llu\n", key, (unsigned long long)value);
 }
 
-static int cgroup_seqfile_show(struct seq_file *m, void *arg)
+static int cgroup_map_seqfile_show(struct seq_file *m, void *arg)
 {
 	struct cgroup_seqfile_state *state = m->private;
 	struct cftype *cft = state->cft;
@@ -1529,6 +1529,13 @@ static int cgroup_seqfile_show(struct seq_file *m, void *arg)
 	return cft->read_map(state->cgroup, cft, &cb);
 }
 
+static int cgroup_simple_seqfile_show(struct seq_file *m, void *arg)
+{
+	struct cgroup_seqfile_state *state = m->private;
+	struct cftype *cft = state->cft;
+	return cft->read_seq(state->cgroup, cft, m);
+}
+
 int cgroup_seqfile_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *seq = file->private_data;
@@ -1562,7 +1569,18 @@ static int cgroup_file_open(struct inode *inode, struct file *file)
 		state->cft = cft;
 		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
 		file->f_op = &cgroup_seqfile_operations;
-		err = single_open(file, cgroup_seqfile_show, state);
+		err = single_open(file, cgroup_map_seqfile_show, state);
+		if (err < 0)
+			kfree(state);
+	} else if (cft->read_seq) {
+		struct cgroup_seqfile_state *state =
+			kzalloc(sizeof(*state), GFP_USER);
+		if (!state)
+			return -ENOMEM;
+		state->cft = cft;
+		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
+		file->f_op = &cgroup_seqfile_operations;
+		err = single_open(file, cgroup_simple_seqfile_show, state);
 		if (err < 0)
 			kfree(state);
 	} else if (cft->open)
diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 4237b19..68a247c 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -9,6 +9,7 @@
 #include <linux/ctype.h>
 #include <linux/list.h>
 #include <linux/uaccess.h>
+#include <linux/seq_file.h>
 
 #define ACC_MKNOD 1
 #define ACC_READ  2
@@ -201,11 +202,15 @@ static void devcgroup_destroy(struct cgroup_subsys *ss,
 
 #define DEVCG_ALLOW 1
 #define DEVCG_DENY 2
+#define DEVCG_LIST 3
+
+#define MAJMINLEN 10
+#define ACCLEN 4
 
 static void set_access(char *acc, short access)
 {
 	int idx = 0;
-	memset(acc, 0, 4);
+	memset(acc, 0, ACCLEN);
 	if (access & ACC_READ)
 		acc[idx++] = 'r';
 	if (access & ACC_WRITE)
@@ -225,70 +230,33 @@ static char type_to_char(short type)
 	return 'X';
 }
 
-static void set_majmin(char *str, int len, unsigned m)
+static void set_majmin(char *str, unsigned m)
 {
-	memset(str, 0, len);
+	memset(str, 0, MAJMINLEN);
 	if (m == ~0)
 		sprintf(str, "*");
 	else
-		snprintf(str, len, "%d", m);
+		snprintf(str, MAJMINLEN, "%d", m);
 }
 
-static char *print_whitelist(struct dev_cgroup *devcgroup, int *len)
+static int devcgroup_seq_read(struct cgroup *cgroup, struct cftype *cft,
+				struct seq_file *m)
 {
-	char *buf, *s, acc[4];
+	struct dev_cgroup *devcgroup = cgroup_to_devcgroup(cgroup);
 	struct dev_whitelist_item *wh;
-	int ret;
-	int count = 0;
-	char maj[10], min[10];
-
-	buf = kmalloc(4096, GFP_KERNEL);
-	if (!buf)
-		return ERR_PTR(-ENOMEM);
-	s = buf;
-	*s = '\0';
-	*len = 0;
+	char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
 
 	spin_lock(&devcgroup->lock);
 	list_for_each_entry(wh, &devcgroup->whitelist, list) {
 		set_access(acc, wh->access);
-		set_majmin(maj, 10, wh->major);
-		set_majmin(min, 10, wh->minor);
-		ret = snprintf(s, 4095-(s-buf), "%c %s:%s %s\n",
-			type_to_char(wh->type), maj, min, acc);
-		if (s+ret >= buf+4095) {
-			kfree(buf);
-			buf = ERR_PTR(-ENOMEM);
-			break;
-		}
-		s += ret;
-		*len += ret;
-		count++;
+		set_majmin(maj, wh->major);
+		set_majmin(min, wh->minor);
+		seq_printf(m, "%c %s:%s %s\n", type_to_char(wh->type),
+			   maj, min, acc);
 	}
 	spin_unlock(&devcgroup->lock);
 
-	return buf;
-}
-
-static ssize_t devcgroup_access_read(struct cgroup *cgroup,
-			struct cftype *cft, struct file *file,
-			char __user *userbuf, size_t nbytes, loff_t *ppos)
-{
-	struct dev_cgroup *devcgroup = cgroup_to_devcgroup(cgroup);
-	int filetype = cft->private;
-	char *buffer;
-	int uninitialized_var(len);
-	int retval;
-
-	if (filetype != DEVCG_ALLOW)
-		return -EINVAL;
-	buffer = print_whitelist(devcgroup, &len);
-	if (IS_ERR(buffer))
-		return PTR_ERR(buffer);
-
-	retval = simple_read_from_buffer(userbuf, nbytes, ppos, buffer, len);
-	kfree(buffer);
-	return retval;
+	return 0;
 }
 
 /*
@@ -501,7 +469,6 @@ out1:
 static struct cftype dev_cgroup_files[] = {
 	{
 		.name = "allow",
-		.read = devcgroup_access_read,
 		.write  = devcgroup_access_write,
 		.private = DEVCG_ALLOW,
 	},
@@ -510,6 +477,11 @@ static struct cftype dev_cgroup_files[] = {
 		.write = devcgroup_access_write,
 		.private = DEVCG_DENY,
 	},
+	{
+		.name = "list",
+		.read_seq = devcgroup_seq_read,
+		.private = DEVCG_LIST,
+	},
 };
 
 static int devcgroup_populate(struct cgroup_subsys *ss,
-- 
1.5.3.6

Re: [PATCH 1/1] cgroups: introduce cft->read_seq()

[Li Zefan]

Serge E. Hallyn wrote:
> Hi Paul,
> 
> the following (against 2.6.25-rc8-mm1) is a first attempt
> at a simple seq_file usage in cgroups.  Comments much
> appreciated.
> 
> thanks,
> -serge
> 
>>From bd0977a5819dc43866fff325ae1e2726e747e2f4 Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <serue@us.ibm.com>
> Date: Wed, 2 Apr 2008 17:54:21 -0700
> Subject: [PATCH 1/1] cgroups: introduce cft->read_seq()
> 
> Introduce a read_seq() helper in cftype, which uses
> seq_file to print out lists.  Use it in the devices
> cgroup.  Also split devices.allow into two files, so
> now devices.deny and devices.allow are the ones to
> use to manipulate the whitelist, while devices.list
> outputs the cgroup's current whitelist.
> 
> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> ---
>  include/linux/cgroup.h   |    6 ++++
>  kernel/cgroup.c          |   22 ++++++++++++-
>  security/device_cgroup.c |   74 ++++++++++++++-------------------------------
>  3 files changed, 49 insertions(+), 53 deletions(-)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 2d1d151..feb83dd 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -227,6 +227,12 @@ struct cftype {
>  	 */
>  	int (*read_map) (struct cgroup *cont, struct cftype *cft,
>  			 struct cgroup_map_cb *cb);
> +	/*
> +	 * read_seq() is used for outputting a simple sequence
> +	 * using seqfile.
> +	 */
> +	int (*read_seq) (struct cgroup *cont, struct cftype *cft,
> +			 struct seq_file *m);
>  

Can't we remove read_map() ?

btw: s/cont/cgrp

>  	ssize_t (*write) (struct cgroup *cgrp, struct cftype *cft,
>  			  struct file *file,
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 85f31ad..61dc509 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1518,7 +1518,7 @@ static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
>  	return seq_printf(sf, "%s %llu\n", key, (unsigned long long)value);
>  }
>  
> -static int cgroup_seqfile_show(struct seq_file *m, void *arg)
> +static int cgroup_map_seqfile_show(struct seq_file *m, void *arg)
>  {
>  	struct cgroup_seqfile_state *state = m->private;
>  	struct cftype *cft = state->cft;
> @@ -1529,6 +1529,13 @@ static int cgroup_seqfile_show(struct seq_file *m, void *arg)
>  	return cft->read_map(state->cgroup, cft, &cb);
>  }
>  
> +static int cgroup_simple_seqfile_show(struct seq_file *m, void *arg)
> +{
> +	struct cgroup_seqfile_state *state = m->private;
> +	struct cftype *cft = state->cft;
> +	return cft->read_seq(state->cgroup, cft, m);
> +}
> +
>  int cgroup_seqfile_release(struct inode *inode, struct file *file)
>  {
>  	struct seq_file *seq = file->private_data;
> @@ -1562,7 +1569,18 @@ static int cgroup_file_open(struct inode *inode, struct file *file)
>  		state->cft = cft;
>  		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
>  		file->f_op = &cgroup_seqfile_operations;
> -		err = single_open(file, cgroup_seqfile_show, state);
> +		err = single_open(file, cgroup_map_seqfile_show, state);
> +		if (err < 0)
> +			kfree(state);
> +	} else if (cft->read_seq) {
> +		struct cgroup_seqfile_state *state =
> +			kzalloc(sizeof(*state), GFP_USER);
> +		if (!state)
> +			return -ENOMEM;
> +		state->cft = cft;
> +		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
> +		file->f_op = &cgroup_seqfile_operations;
> +		err = single_open(file, cgroup_simple_seqfile_show, state);
>  		if (err < 0)
>  			kfree(state);
>  	} else if (cft->open)
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index 4237b19..68a247c 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -9,6 +9,7 @@
>  #include <linux/ctype.h>
>  #include <linux/list.h>
>  #include <linux/uaccess.h>
> +#include <linux/seq_file.h>
>  
>  #define ACC_MKNOD 1
>  #define ACC_READ  2
> @@ -201,11 +202,15 @@ static void devcgroup_destroy(struct cgroup_subsys *ss,
>  
>  #define DEVCG_ALLOW 1
>  #define DEVCG_DENY 2
> +#define DEVCG_LIST 3
> +
> +#define MAJMINLEN 10
> +#define ACCLEN 4
>  
>  static void set_access(char *acc, short access)
>  {
>  	int idx = 0;
> -	memset(acc, 0, 4);
> +	memset(acc, 0, ACCLEN);
>  	if (access & ACC_READ)
>  		acc[idx++] = 'r';
>  	if (access & ACC_WRITE)
> @@ -225,70 +230,33 @@ static char type_to_char(short type)
>  	return 'X';
>  }
>  
> -static void set_majmin(char *str, int len, unsigned m)
> +static void set_majmin(char *str, unsigned m)
>  {
> -	memset(str, 0, len);
> +	memset(str, 0, MAJMINLEN);
>  	if (m == ~0)
>  		sprintf(str, "*");
>  	else
> -		snprintf(str, len, "%d", m);
> +		snprintf(str, MAJMINLEN, "%d", m);
>  }
>  
> -static char *print_whitelist(struct dev_cgroup *devcgroup, int *len)
> +static int devcgroup_seq_read(struct cgroup *cgroup, struct cftype *cft,
> +				struct seq_file *m)
>  {
> -	char *buf, *s, acc[4];
> +	struct dev_cgroup *devcgroup = cgroup_to_devcgroup(cgroup);
>  	struct dev_whitelist_item *wh;
> -	int ret;
> -	int count = 0;
> -	char maj[10], min[10];
> -
> -	buf = kmalloc(4096, GFP_KERNEL);
> -	if (!buf)
> -		return ERR_PTR(-ENOMEM);
> -	s = buf;
> -	*s = '\0';
> -	*len = 0;
> +	char maj[MAJMINLEN], min[MAJMINLEN], acc[ACCLEN];
>  
>  	spin_lock(&devcgroup->lock);
>  	list_for_each_entry(wh, &devcgroup->whitelist, list) {
>  		set_access(acc, wh->access);
> -		set_majmin(maj, 10, wh->major);
> -		set_majmin(min, 10, wh->minor);
> -		ret = snprintf(s, 4095-(s-buf), "%c %s:%s %s\n",
> -			type_to_char(wh->type), maj, min, acc);
> -		if (s+ret >= buf+4095) {
> -			kfree(buf);
> -			buf = ERR_PTR(-ENOMEM);
> -			break;
> -		}
> -		s += ret;
> -		*len += ret;
> -		count++;
> +		set_majmin(maj, wh->major);
> +		set_majmin(min, wh->minor);
> +		seq_printf(m, "%c %s:%s %s\n", type_to_char(wh->type),
> +			   maj, min, acc);
>  	}
>  	spin_unlock(&devcgroup->lock);
>  
> -	return buf;
> -}
> -
> -static ssize_t devcgroup_access_read(struct cgroup *cgroup,
> -			struct cftype *cft, struct file *file,
> -			char __user *userbuf, size_t nbytes, loff_t *ppos)
> -{
> -	struct dev_cgroup *devcgroup = cgroup_to_devcgroup(cgroup);
> -	int filetype = cft->private;
> -	char *buffer;
> -	int uninitialized_var(len);
> -	int retval;
> -
> -	if (filetype != DEVCG_ALLOW)
> -		return -EINVAL;
> -	buffer = print_whitelist(devcgroup, &len);
> -	if (IS_ERR(buffer))
> -		return PTR_ERR(buffer);
> -
> -	retval = simple_read_from_buffer(userbuf, nbytes, ppos, buffer, len);
> -	kfree(buffer);
> -	return retval;
> +	return 0;
>  }
>  
>  /*
> @@ -501,7 +469,6 @@ out1:
>  static struct cftype dev_cgroup_files[] = {
>  	{
>  		.name = "allow",
> -		.read = devcgroup_access_read,
>  		.write  = devcgroup_access_write,
>  		.private = DEVCG_ALLOW,
>  	},
> @@ -510,6 +477,11 @@ static struct cftype dev_cgroup_files[] = {
>  		.write = devcgroup_access_write,
>  		.private = DEVCG_DENY,
>  	},
> +	{
> +		.name = "list",
> +		.read_seq = devcgroup_seq_read,
> +		.private = DEVCG_LIST,
> +	},
>  };
>  
>  static int devcgroup_populate(struct cgroup_subsys *ss,

Re: [PATCH 1/1] cgroups: introduce cft->read_seq()

[Li Zefan]

Li Zefan wrote:
> Serge E. Hallyn wrote:
>> Hi Paul,
>>
>> the following (against 2.6.25-rc8-mm1) is a first attempt
>> at a simple seq_file usage in cgroups.  Comments much
>> appreciated.
>>
>> thanks,
>> -serge
>>
>> >From bd0977a5819dc43866fff325ae1e2726e747e2f4 Mon Sep 17 00:00:00 2001
>> From: Serge E. Hallyn <serue@us.ibm.com>
>> Date: Wed, 2 Apr 2008 17:54:21 -0700
>> Subject: [PATCH 1/1] cgroups: introduce cft->read_seq()
>>
>> Introduce a read_seq() helper in cftype, which uses
>> seq_file to print out lists.  Use it in the devices
>> cgroup.  Also split devices.allow into two files, so
>> now devices.deny and devices.allow are the ones to
>> use to manipulate the whitelist, while devices.list
>> outputs the cgroup's current whitelist.
>>
>> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
>> ---
>>  include/linux/cgroup.h   |    6 ++++
>>  kernel/cgroup.c          |   22 ++++++++++++-
>>  security/device_cgroup.c |   74 ++++++++++++++-------------------------------
>>  3 files changed, 49 insertions(+), 53 deletions(-)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 2d1d151..feb83dd 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -227,6 +227,12 @@ struct cftype {
>>  	 */
>>  	int (*read_map) (struct cgroup *cont, struct cftype *cft,
>>  			 struct cgroup_map_cb *cb);
>> +	/*
>> +	 * read_seq() is used for outputting a simple sequence
>> +	 * using seqfile.
>> +	 */
>> +	int (*read_seq) (struct cgroup *cont, struct cftype *cft,
>> +			 struct seq_file *m);
>>  
> 
> Can't we remove read_map() ?
> 
> btw: s/cont/cgrp
> 

It seems read_seq() is the general case of read_map(). A read_seq() method can
produce the output freely, and read_map() strictly produces string->u64 maps.

Re: [PATCH 1/1] cgroups: introduce cft->read_seq()

[Serge E. Hallyn]

Quoting Li Zefan (lizf@cn.fujitsu.com):
> Li Zefan wrote:
> > Serge E. Hallyn wrote:
> >> Hi Paul,
> >>
> >> the following (against 2.6.25-rc8-mm1) is a first attempt
> >> at a simple seq_file usage in cgroups.  Comments much
> >> appreciated.
> >>
> >> thanks,
> >> -serge
> >>
> >> >From bd0977a5819dc43866fff325ae1e2726e747e2f4 Mon Sep 17 00:00:00 2001
> >> From: Serge E. Hallyn <serue@us.ibm.com>
> >> Date: Wed, 2 Apr 2008 17:54:21 -0700
> >> Subject: [PATCH 1/1] cgroups: introduce cft->read_seq()
> >>
> >> Introduce a read_seq() helper in cftype, which uses
> >> seq_file to print out lists.  Use it in the devices
> >> cgroup.  Also split devices.allow into two files, so
> >> now devices.deny and devices.allow are the ones to
> >> use to manipulate the whitelist, while devices.list
> >> outputs the cgroup's current whitelist.
> >>
> >> Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> >> ---
> >>  include/linux/cgroup.h   |    6 ++++
> >>  kernel/cgroup.c          |   22 ++++++++++++-
> >>  security/device_cgroup.c |   74 ++++++++++++++-------------------------------
> >>  3 files changed, 49 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> >> index 2d1d151..feb83dd 100644
> >> --- a/include/linux/cgroup.h
> >> +++ b/include/linux/cgroup.h
> >> @@ -227,6 +227,12 @@ struct cftype {
> >>  	 */
> >>  	int (*read_map) (struct cgroup *cont, struct cftype *cft,
> >>  			 struct cgroup_map_cb *cb);
> >> +	/*
> >> +	 * read_seq() is used for outputting a simple sequence
> >> +	 * using seqfile.
> >> +	 */
> >> +	int (*read_seq) (struct cgroup *cont, struct cftype *cft,
> >> +			 struct seq_file *m);
> >>  
> > 
> > Can't we remove read_map() ?
> > 
> > btw: s/cont/cgrp
> > 
> 
> It seems read_seq() is the general case of read_map(). A read_seq() method can
> produce the output freely, and read_map() strictly produces string->u64 maps.

Right, the memcontroller cgroup is using read_map(), and I seem to recall
it was intended to be of pretty general use so I was going to just
leave it there.

As for s/cont/cgrp I agree but that's for a separate pure renaming
patch.  In the meantime I just wanted to follow the existing style.
I could rebase this patch on top of a renaming patch but that's
definately wrong for just soliciting comments on this being the right
approach.

Thanks for taking a look.

-serge

Re: [PATCH 1/1] cgroups: introduce cft->read_seq()

[Paul Menage]

On Wed, Apr 2, 2008 at 6:01 PM, Serge E. Hallyn <serue@us.ibm.com> wrote:
>  +       /*
>  +        * read_seq() is used for outputting a simple sequence
>  +        * using seqfile.
>  +        */
>  +       int (*read_seq) (struct cgroup *cont, struct cftype *cft,
>  +                        struct seq_file *m);

Maybe make it read_seq_string() to emphasise that it's not intended
for structured data?

>
>  -static int cgroup_seqfile_show(struct seq_file *m, void *arg)
>  +static int cgroup_map_seqfile_show(struct seq_file *m, void *arg)
>   {
>         struct cgroup_seqfile_state *state = m->private;
>         struct cftype *cft = state->cft;
>  @@ -1529,6 +1529,13 @@ static int cgroup_seqfile_show(struct seq_file *m, void *arg)
>         return cft->read_map(state->cgroup, cft, &cb);
>   }

I think you can make this simpler - have cgroup_seqfile_show() switch
based on whether the cft has a read_seq_string or a read_map, and then
you only need one function.

>
>  +static int cgroup_simple_seqfile_show(struct seq_file *m, void *arg)
>  +{
>  +       struct cgroup_seqfile_state *state = m->private;
>  +       struct cftype *cft = state->cft;
>  +       return cft->read_seq(state->cgroup, cft, m);
>  +}
>  +
>   int cgroup_seqfile_release(struct inode *inode, struct file *file)
>   {
>         struct seq_file *seq = file->private_data;
>  @@ -1562,7 +1569,18 @@ static int cgroup_file_open(struct inode *inode, struct file *file)
>                 state->cft = cft;
>                 state->cgroup = __d_cgrp(file->f_dentry->d_parent);
>                 file->f_op = &cgroup_seqfile_operations;
>  -               err = single_open(file, cgroup_seqfile_show, state);
>  +               err = single_open(file, cgroup_map_seqfile_show, state);
>  +               if (err < 0)
>  +                       kfree(state);
>  +       } else if (cft->read_seq) {
>  +               struct cgroup_seqfile_state *state =
>  +                       kzalloc(sizeof(*state), GFP_USER);
>  +               if (!state)
>  +                       return -ENOMEM;
>  +               state->cft = cft;
>  +               state->cgroup = __d_cgrp(file->f_dentry->d_parent);
>  +               file->f_op = &cgroup_seqfile_operations;
>  +               err = single_open(file, cgroup_simple_seqfile_show, state);
>                 if (err < 0)
>                         kfree(state);

and this arm can just be eliminated, by adding "||
cft->read_seq_string" to the test for cft_read_seq_map above.

It was always my intention that cgroup_seqfile_show() be able to
handle multiple data types, but up until now we only had one.

Paul

Re: [PATCH 1/1] cgroups: introduce cft->read_seq()

[serge]

Quoting Paul Menage (menage@google.com):
> On Wed, Apr 2, 2008 at 6:01 PM, Serge E. Hallyn <serue@us.ibm.com> wrote:
> >  +       /*
> >  +        * read_seq() is used for outputting a simple sequence
> >  +        * using seqfile.
> >  +        */
> >  +       int (*read_seq) (struct cgroup *cont, struct cftype *cft,
> >  +                        struct seq_file *m);
> 
> Maybe make it read_seq_string() to emphasise that it's not intended
> for structured data?
> 
> >
> >  -static int cgroup_seqfile_show(struct seq_file *m, void *arg)
> >  +static int cgroup_map_seqfile_show(struct seq_file *m, void *arg)
> >   {
> >         struct cgroup_seqfile_state *state = m->private;
> >         struct cftype *cft = state->cft;
> >  @@ -1529,6 +1529,13 @@ static int cgroup_seqfile_show(struct seq_file *m, void *arg)
> >         return cft->read_map(state->cgroup, cft, &cb);
> >   }
> 
> I think you can make this simpler - have cgroup_seqfile_show() switch
> based on whether the cft has a read_seq_string or a read_map, and then
> you only need one function.
> 
> >
> >  +static int cgroup_simple_seqfile_show(struct seq_file *m, void *arg)
> >  +{
> >  +       struct cgroup_seqfile_state *state = m->private;
> >  +       struct cftype *cft = state->cft;
> >  +       return cft->read_seq(state->cgroup, cft, m);
> >  +}
> >  +
> >   int cgroup_seqfile_release(struct inode *inode, struct file *file)
> >   {
> >         struct seq_file *seq = file->private_data;
> >  @@ -1562,7 +1569,18 @@ static int cgroup_file_open(struct inode *inode, struct file *file)
> >                 state->cft = cft;
> >                 state->cgroup = __d_cgrp(file->f_dentry->d_parent);
> >                 file->f_op = &cgroup_seqfile_operations;
> >  -               err = single_open(file, cgroup_seqfile_show, state);
> >  +               err = single_open(file, cgroup_map_seqfile_show, state);
> >  +               if (err < 0)
> >  +                       kfree(state);
> >  +       } else if (cft->read_seq) {
> >  +               struct cgroup_seqfile_state *state =
> >  +                       kzalloc(sizeof(*state), GFP_USER);
> >  +               if (!state)
> >  +                       return -ENOMEM;
> >  +               state->cft = cft;
> >  +               state->cgroup = __d_cgrp(file->f_dentry->d_parent);
> >  +               file->f_op = &cgroup_seqfile_operations;
> >  +               err = single_open(file, cgroup_simple_seqfile_show, state);
> >                 if (err < 0)
> >                         kfree(state);
> 
> and this arm can just be eliminated, by adding "||
> cft->read_seq_string" to the test for cft_read_seq_map above.
> 
> It was always my intention that cgroup_seqfile_show() be able to
> handle multiple data types, but up until now we only had one.

Thanks Paul, will do each of these.

-serge