[PATCH 1/1] cls_cgroup: unify classid syntax to tc

[PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

cls_cgroup: unify classid syntax to tc

This patch unifies classid syntax of cls_cgroup to tc.
In present implementation of cls_cgroup, classid read/write syntax is different from tc's one.

  tc     1:10
  write  0x10010 (hexadecimal)
  read   65552   (decimal)

-------------------------------------------------
  # echo 0x10010 > /cgroup/net/net_cls.classid
  # cat /cgroup/net/net_cls.classid
  65552
-------------------------------------------------

This is very inconvenient, so I made a patch which enables only tc syntax.
If we don't classify with net_cls.classid, we write 0 to net_cls.classid.
(This behavior is the same as present implementation.)

-------------------------------------------------
  # echo 1:10 > /cgroup/net/net_cls.classid
  # cat /cgroup/net/net_cls.classid
  1:10
  # echo 0 > /cgroup/net/net_cls.classid
  # cat /cgroup/net/net_cls.classid
  0
-------------------------------------------------


Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp>

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index e5becb9..6d87d00 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/string.h>
+#include <linux/ctype.h>
 #include <linux/errno.h>
 #include <linux/skbuff.h>
 #include <linux/cgroup.h>
@@ -55,17 +56,76 @@ static void cgrp_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp)
 	kfree(cgrp_cls_state(cgrp));
 }
 
-static u64 read_classid(struct cgroup *cgrp, struct cftype *cft)
+#define CLS_CGROUP_CLASSID_LEN 16
+
+static ssize_t read_classid(struct cgroup *cgrp, struct cftype *cft,
+				struct file *file, char __user *buf,
+				size_t nbytes, loff_t *ppos)
+{
+	u32 value;
+	char classid[CLS_CGROUP_CLASSID_LEN];
+
+	value = cgrp_cls_state(cgrp)->classid;
+	if (value)
+		sprintf(classid, "%x:%x\n",
+			TC_H_MAJ(value)>>16, TC_H_MIN(value));
+	else
+		strcpy(classid, "0\n");
+
+	return simple_read_from_buffer(buf, nbytes, ppos,
+					classid, strlen(classid));
+}
+
+static int get_tc_classid(u32 *classid, const char *str)
 {
-	return cgrp_cls_state(cgrp)->classid;
+	u32 maj, min;
+	char *p;
+
+	/* check "0" for reset request */
+	if (!strcmp(str, "0")) {
+		*classid = 0;
+		return 0;
+	}
+
+	/* parse major number */
+	maj = simple_strtoul(str, &p, 16);
+	if (p == str) {
+		maj = 0;
+		if (*p != ':')
+			return -EINVAL;
+	}
+
+	/* parse minor number */
+	if (*p == ':') {
+		if (maj >= (1<<16))
+			return -EINVAL;
+
+		str = p + 1;
+		min = simple_strtoul(str, &p, 16);
+		if (*p != 0)
+			return -EINVAL;
+		if (min >= (1<<16))
+			return -EINVAL;
+	} else if (*p != 0)
+		return -EINVAL;
+
+	*classid = TC_H_MAKE(maj<<16, min);
+
+	return 0;
 }
 
-static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value)
+static int write_classid(struct cgroup *cgrp, struct cftype *cft,
+				const char *buf)
 {
+	u32 classid;
+
 	if (!cgroup_lock_live_group(cgrp))
 		return -ENODEV;
 
-	cgrp_cls_state(cgrp)->classid = (u32) value;
+	if (get_tc_classid(&classid, buf) < 0)
+		return -EINVAL;
+
+	cgrp_cls_state(cgrp)->classid = classid;
 
 	cgroup_unlock();
 
@@ -75,8 +135,8 @@ static int write_classid(struct cgroup *cgrp, struct cftype *cft, u64 value)
 static struct cftype ss_files[] = {
 	{
 		.name = "classid",
-		.read_u64 = read_classid,
-		.write_u64 = write_classid,
+		.read = read_classid,
+		.write_string = write_classid,
 	},
 };
 

-- 
Minoru Usui <usui@mxm.nes.nec.co.jp>

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[David Miller]

From: Minoru Usui <usui@mxm.nes.nec.co.jp>
Date: Thu, 11 Jun 2009 18:05:55 +0900

> This patch unifies classid syntax of cls_cgroup to tc.

Won't this break existing users?

We really can't say "oh sorry" and change things after it's
been effectively deployed already.

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

Hi, David

Thank you for your comments.

On Thu, 11 Jun 2009 02:13:47 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Minoru Usui <usui@mxm.nes.nec.co.jp>
> Date: Thu, 11 Jun 2009 18:05:55 +0900
> 
> > This patch unifies classid syntax of cls_cgroup to tc.
> 
> Won't this break existing users?
> 
> We really can't say "oh sorry" and change things after it's
> been effectively deployed already.

It's true. If my patch applied, old syntax can't use.
But I think compatibility is important, too.
So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file.

Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal)
I think this might be interface bug, so I think we should unify to read/write syntax. 
Of course, if read syntax change from decimal expression, there isn't compatibility.
But I think it's better to fix this problem. 
What do you think?

If it's ok, I'd like to be able to use tc syntax(X:Y style),
because cls_cgroup's purpose is select class which create by tc command.
tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup. 
-- 
Minoru Usui <usui@mxm.nes.nec.co.jp>

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[David Miller]

From: Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>
Date: Fri, 12 Jun 2009 12:40:28 +0900

> Of course, if read syntax change from decimal expression, there
> isn't compatibility.  But I think it's better to fix this problem.
> What do you think?

I think compatibility is more important than symmetry.

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Paul Menage]

On Thu, Jun 11, 2009 at 8:40 PM, Minoru Usui<usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org> wrote:
>
> It's true. If my patch applied, old syntax can't use.
> But I think compatibility is important, too.
> So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file.
>
> Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal)

You can write the classid value in decimal too, or even octal - it
just uses strtoull() to convert the string to an integer, so
0x-prefixed hex values are handled automatically.

>
> If it's ok, I'd like to be able to use tc syntax(X:Y style),
> because cls_cgroup's purpose is select class which create by tc command.
> tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup.

We should keep it as it is - purely apart from compatibility issues,
there's no need to introduce the complexity of the tc interface into
the cls_cgroup kernel code. When a cgroup control file represents a
simple integer, then it should be exposed as such.

Paul

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[David Miller]

From: Minoru Usui <usui@mxm.nes.nec.co.jp>
Date: Fri, 12 Jun 2009 12:40:28 +0900

> Of course, if read syntax change from decimal expression, there
> isn't compatibility.  But I think it's better to fix this problem.
> What do you think?

I think compatibility is more important than symmetry.

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Paul Menage]

On Thu, Jun 11, 2009 at 8:40 PM, Minoru Usui<usui@mxm.nes.nec.co.jp> wrote:
>
> It's true. If my patch applied, old syntax can't use.
> But I think compatibility is important, too.
> So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file.
>
> Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal)

You can write the classid value in decimal too, or even octal - it
just uses strtoull() to convert the string to an integer, so
0x-prefixed hex values are handled automatically.

>
> If it's ok, I'd like to be able to use tc syntax(X:Y style),
> because cls_cgroup's purpose is select class which create by tc command.
> tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup.

We should keep it as it is - purely apart from compatibility issues,
there's no need to introduce the complexity of the tc interface into
the cls_cgroup kernel code. When a cgroup control file represents a
simple integer, then it should be exposed as such.

Paul

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

Hi, Paul.

Thank you for your advice.

On Thu, 11 Jun 2009 22:06:00 -0700
Paul Menage <menage-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

> On Thu, Jun 11, 2009 at 8:40 PM, Minoru Usui<usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org> wrote:
> >
> > It's true. If my patch applied, old syntax can't use.
> > But I think compatibility is important, too.
> > So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file.
> >
> > Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal)
> 
> You can write the classid value in decimal too, or even octal - it
> just uses strtoull() to convert the string to an integer, so
> 0x-prefixed hex values are handled automatically.
> 
> >
> > If it's ok, I'd like to be able to use tc syntax(X:Y style),
> > because cls_cgroup's purpose is select class which create by tc command.
> > tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup.
> 
> We should keep it as it is - purely apart from compatibility issues,
> there's no need to introduce the complexity of the tc interface into
> the cls_cgroup kernel code. When a cgroup control file represents a
> simple integer, then it should be exposed as such.

I understand your opinion.
You mean, kernel I/F should be simple, so we should use the same format as the kernel.
I completely agree.

Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
X:Y style is result of translating by tc command.

I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
This function can be implemented on user space if we need.
And we can also keep compatibility of net_cls.classid I/F. :-)

I drop this patch. I'm sorry for confusing a lot of people.
-- 
Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Thomas Graf]

On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
> X:Y style is result of translating by tc command.
> 
> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
> This function can be implemented on user space if we need.
> And we can also keep compatibility of net_cls.classid I/F. :-)
> 
> I drop this patch. I'm sorry for confusing a lot of people.

I found this patch to be extremely useful if it wasn't to break
compatibility but that issue can be resolved by accepting both
formats easly.

A classid is in a fact a set of two 16 bit integers and everyone
is used to the X:Y notation. procfs and sysfs ought to be userfriendly
to a certain degree and adding such a small parser to cls_cgroup
doesn't harm anyone.

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Thomas Graf]

On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
> X:Y style is result of translating by tc command.
> 
> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
> This function can be implemented on user space if we need.
> And we can also keep compatibility of net_cls.classid I/F. :-)
> 
> I drop this patch. I'm sorry for confusing a lot of people.

I found this patch to be extremely useful if it wasn't to break
compatibility but that issue can be resolved by accepting both
formats easly.

A classid is in a fact a set of two 16 bit integers and everyone
is used to the X:Y notation. procfs and sysfs ought to be userfriendly
to a certain degree and adding such a small parser to cls_cgroup
doesn't harm anyone.

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

On Mon, 15 Jun 2009 06:19:19 -0400
Thomas Graf <tgraf-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
> > Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
> > X:Y style is result of translating by tc command.
> > 
> > I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
> > This function can be implemented on user space if we need.
> > And we can also keep compatibility of net_cls.classid I/F. :-)
> > 
> > I drop this patch. I'm sorry for confusing a lot of people.
> 
> I found this patch to be extremely useful if it wasn't to break
> compatibility but that issue can be resolved by accepting both
> formats easly.

Thank you for agreeing my patch.

I think you said write format only. Am I right?
What do you think about read format?
Do you think you should keep read format as current implementation?

I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. 
Because some of people are confused about having to read their write value as different format.
(Unfortunately, current implementation is so...)

But this approach breaks compatibility, so I drop this patch.
-- 
Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

On Mon, 15 Jun 2009 06:19:19 -0400
Thomas Graf <tgraf@infradead.org> wrote:

> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
> > Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
> > X:Y style is result of translating by tc command.
> > 
> > I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
> > This function can be implemented on user space if we need.
> > And we can also keep compatibility of net_cls.classid I/F. :-)
> > 
> > I drop this patch. I'm sorry for confusing a lot of people.
> 
> I found this patch to be extremely useful if it wasn't to break
> compatibility but that issue can be resolved by accepting both
> formats easly.

Thank you for agreeing my patch.

I think you said write format only. Am I right?
What do you think about read format?
Do you think you should keep read format as current implementation?

I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. 
Because some of people are confused about having to read their write value as different format.
(Unfortunately, current implementation is so...)

But this approach breaks compatibility, so I drop this patch.
-- 
Minoru Usui <usui@mxm.nes.nec.co.jp>

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Li Zefan]

Minoru Usui wrote:
> On Mon, 15 Jun 2009 06:19:19 -0400
> Thomas Graf <tgraf@infradead.org> wrote:
> 
>> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
>>> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
>>> X:Y style is result of translating by tc command.
>>>
>>> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
>>> This function can be implemented on user space if we need.
>>> And we can also keep compatibility of net_cls.classid I/F. :-)
>>>
>>> I drop this patch. I'm sorry for confusing a lot of people.
>> I found this patch to be extremely useful if it wasn't to break
>> compatibility but that issue can be resolved by accepting both
>> formats easly.
> 
> Thank you for agreeing my patch.
> 
> I think you said write format only. Am I right?
> What do you think about read format?
> Do you think you should keep read format as current implementation?
> 
> I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. 
> Because some of people are confused about having to read their write value as different format.
> (Unfortunately, current implementation is so...)
> 
> But this approach breaks compatibility, so I drop this patch.

How about adding a new control file net_cls.tc_classid?

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

On Thu, 18 Jun 2009 10:01:27 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> Minoru Usui wrote:
> > On Mon, 15 Jun 2009 06:19:19 -0400
> > Thomas Graf <tgraf@infradead.org> wrote:
> > 
> >> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
> >>> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
> >>> X:Y style is result of translating by tc command.
> >>>
> >>> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
> >>> This function can be implemented on user space if we need.
> >>> And we can also keep compatibility of net_cls.classid I/F. :-)
> >>>
> >>> I drop this patch. I'm sorry for confusing a lot of people.
> >> I found this patch to be extremely useful if it wasn't to break
> >> compatibility but that issue can be resolved by accepting both
> >> formats easly.
> > 
> > Thank you for agreeing my patch.
> > 
> > I think you said write format only. Am I right?
> > What do you think about read format?
> > Do you think you should keep read format as current implementation?
> > 
> > I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. 
> > Because some of people are confused about having to read their write value as different format.
> > (Unfortunately, current implementation is so...)
> > 
> > But this approach breaks compatibility, so I drop this patch.
> 
> How about adding a new control file net_cls.tc_classid?
> 

net_cls.classid's core purpose is to read/write classid. 
If we add net_cls.tc_classid, its core purpose is same.
Their difference is only read/write value of expression.

Is this approach ok?

-- 
Minoru Usui <usui@mxm.nes.nec.co.jp>

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Paul Menage]

On Wed, Jun 17, 2009 at 11:15 PM, Minoru Usui<usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org> wrote:
>
> net_cls.classid's core purpose is to read/write classid.
> If we add net_cls.tc_classid, its core purpose is same.
> Their difference is only read/write value of expression.

I still feel that it's unnecessary bloat to put that parsing/printing
logic in the kernel when userspace can do it just as easily. But
having it as a separate file would at least remove the compatibility
concerns.

Paul

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Paul Menage]

On Wed, Jun 17, 2009 at 11:15 PM, Minoru Usui<usui@mxm.nes.nec.co.jp> wrote:
>
> net_cls.classid's core purpose is to read/write classid.
> If we add net_cls.tc_classid, its core purpose is same.
> Their difference is only read/write value of expression.

I still feel that it's unnecessary bloat to put that parsing/printing
logic in the kernel when userspace can do it just as easily. But
having it as a separate file would at least remove the compatibility
concerns.

Paul

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

On Thu, 18 Jun 2009 10:01:27 +0800
Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> wrote:

> Minoru Usui wrote:
> > On Mon, 15 Jun 2009 06:19:19 -0400
> > Thomas Graf <tgraf-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> > 
> >> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
> >>> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
> >>> X:Y style is result of translating by tc command.
> >>>
> >>> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
> >>> This function can be implemented on user space if we need.
> >>> And we can also keep compatibility of net_cls.classid I/F. :-)
> >>>
> >>> I drop this patch. I'm sorry for confusing a lot of people.
> >> I found this patch to be extremely useful if it wasn't to break
> >> compatibility but that issue can be resolved by accepting both
> >> formats easly.
> > 
> > Thank you for agreeing my patch.
> > 
> > I think you said write format only. Am I right?
> > What do you think about read format?
> > Do you think you should keep read format as current implementation?
> > 
> > I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. 
> > Because some of people are confused about having to read their write value as different format.
> > (Unfortunately, current implementation is so...)
> > 
> > But this approach breaks compatibility, so I drop this patch.
> 
> How about adding a new control file net_cls.tc_classid?
> 

net_cls.classid's core purpose is to read/write classid. 
If we add net_cls.tc_classid, its core purpose is same.
Their difference is only read/write value of expression.

Is this approach ok?

-- 
Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>

Re: [Devel] Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Li Zefan]

Minoru Usui wrote:
> On Mon, 15 Jun 2009 06:19:19 -0400
> Thomas Graf <tgraf-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> 
>> On Mon, Jun 15, 2009 at 12:00:35PM +0900, Minoru Usui wrote:
>>> Actually, In tc <-> kernel I/F (which uses netlink), tc sets classid to hexadecimal style not X:Y style.
>>> X:Y style is result of translating by tc command.
>>>
>>> I thought this patch was very useful at first, but it's not necessary to implementing to the kernel.
>>> This function can be implemented on user space if we need.
>>> And we can also keep compatibility of net_cls.classid I/F. :-)
>>>
>>> I drop this patch. I'm sorry for confusing a lot of people.
>> I found this patch to be extremely useful if it wasn't to break
>> compatibility but that issue can be resolved by accepting both
>> formats easly.
> 
> Thank you for agreeing my patch.
> 
> I think you said write format only. Am I right?
> What do you think about read format?
> Do you think you should keep read format as current implementation?
> 
> I think we should need to unify read/write format, if we make cls_cgroup is more userfriendly. 
> Because some of people are confused about having to read their write value as different format.
> (Unfortunately, current implementation is so...)
> 
> But this approach breaks compatibility, so I drop this patch.

How about adding a new control file net_cls.tc_classid?

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

Hi, David

Thank you for your comments.

On Thu, 11 Jun 2009 02:13:47 -0700 (PDT)
David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:

> From: Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>
> Date: Thu, 11 Jun 2009 18:05:55 +0900
> 
> > This patch unifies classid syntax of cls_cgroup to tc.
> 
> Won't this break existing users?
> 
> We really can't say "oh sorry" and change things after it's
> been effectively deployed already.

It's true. If my patch applied, old syntax can't use.
But I think compatibility is important, too.
So I'll make a new patch which can use both old and new syntax, when we write classid to net_cls.classid file.

Present implementation is an asymmetry with read and write.(read: decimal, write: hexadecimal)
I think this might be interface bug, so I think we should unify to read/write syntax. 
Of course, if read syntax change from decimal expression, there isn't compatibility.
But I think it's better to fix this problem. 
What do you think?

If it's ok, I'd like to be able to use tc syntax(X:Y style),
because cls_cgroup's purpose is select class which create by tc command.
tc command can only use classid syntax X:Y style, so we should be able to use X:Y style to cls_cgroup. 
-- 
Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[David Miller]

From: Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>
Date: Thu, 11 Jun 2009 18:05:55 +0900

> This patch unifies classid syntax of cls_cgroup to tc.

Won't this break existing users?

We really can't say "oh sorry" and change things after it's
been effectively deployed already.

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Thomas Graf]

Very nice, this is a big improvement.

On Thu, Jun 11, 2009 at 06:05:55PM +0900, Minoru Usui wrote:
> +static int get_tc_classid(u32 *classid, const char *str)

Maybe make this return u32?

>  {
> -	return cgrp_cls_state(cgrp)->classid;
> +	u32 maj, min;
> +	char *p;
> +
> +	/* check "0" for reset request */
> +	if (!strcmp(str, "0")) {
> +		*classid = 0;
> +		return 0;
> +	}
> +
> +	/* parse major number */
> +	maj = simple_strtoul(str, &p, 16);
> +	if (p == str) {
> +		maj = 0;
> +		if (*p != ':')
> +			return -EINVAL;
> +	}
> +
> +	/* parse minor number */
> +	if (*p == ':') {
> +		if (maj >= (1<<16))
> +			return -EINVAL;
> +
> +		str = p + 1;
> +		min = simple_strtoul(str, &p, 16);
> +		if (*p != 0)
> +			return -EINVAL;
> +		if (min >= (1<<16))
> +			return -EINVAL;
> +	} else if (*p != 0)
> +		return -EINVAL;

What do you think about keeping things backwards compatible by
accepting both the X:Y and XY format?

} else if (*p != 0)
	return strtoul(str, NULL, 0);

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Thomas Graf]

Very nice, this is a big improvement.

On Thu, Jun 11, 2009 at 06:05:55PM +0900, Minoru Usui wrote:
> +static int get_tc_classid(u32 *classid, const char *str)

Maybe make this return u32?

>  {
> -	return cgrp_cls_state(cgrp)->classid;
> +	u32 maj, min;
> +	char *p;
> +
> +	/* check "0" for reset request */
> +	if (!strcmp(str, "0")) {
> +		*classid = 0;
> +		return 0;
> +	}
> +
> +	/* parse major number */
> +	maj = simple_strtoul(str, &p, 16);
> +	if (p == str) {
> +		maj = 0;
> +		if (*p != ':')
> +			return -EINVAL;
> +	}
> +
> +	/* parse minor number */
> +	if (*p == ':') {
> +		if (maj >= (1<<16))
> +			return -EINVAL;
> +
> +		str = p + 1;
> +		min = simple_strtoul(str, &p, 16);
> +		if (*p != 0)
> +			return -EINVAL;
> +		if (min >= (1<<16))
> +			return -EINVAL;
> +	} else if (*p != 0)
> +		return -EINVAL;

What do you think about keeping things backwards compatible by
accepting both the X:Y and XY format?

} else if (*p != 0)
	return strtoul(str, NULL, 0);

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

Hi, Thomas

Thank you for your comments.

On Thu, 11 Jun 2009 05:31:49 -0400
Thomas Graf <tgraf-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> Very nice, this is a big improvement.
> 
> On Thu, Jun 11, 2009 at 06:05:55PM +0900, Minoru Usui wrote:
> > +static int get_tc_classid(u32 *classid, const char *str)
> 
> Maybe make this return u32?

Yes, you are right.
I'll fix it.

> 
> >  {
> > -	return cgrp_cls_state(cgrp)->classid;
> > +	u32 maj, min;
> > +	char *p;
> > +
> > +	/* check "0" for reset request */
> > +	if (!strcmp(str, "0")) {
> > +		*classid = 0;
> > +		return 0;
> > +	}
> > +
> > +	/* parse major number */
> > +	maj = simple_strtoul(str, &p, 16);
> > +	if (p == str) {
> > +		maj = 0;
> > +		if (*p != ':')
> > +			return -EINVAL;
> > +	}
> > +
> > +	/* parse minor number */
> > +	if (*p == ':') {
> > +		if (maj >= (1<<16))
> > +			return -EINVAL;
> > +
> > +		str = p + 1;
> > +		min = simple_strtoul(str, &p, 16);
> > +		if (*p != 0)
> > +			return -EINVAL;
> > +		if (min >= (1<<16))
> > +			return -EINVAL;
> > +	} else if (*p != 0)
> > +		return -EINVAL;
> 
> What do you think about keeping things backwards compatible by
> accepting both the X:Y and XY format?
> 
> } else if (*p != 0)
> 	return strtoul(str, NULL, 0);

OK. I'll do it.
-- 
Minoru Usui <usui-vEbCzmPPqHe45+QrQBaojngSJqDPrsil@public.gmane.org>

Re: [PATCH 1/1] cls_cgroup: unify classid syntax to tc

[Minoru Usui]

Hi, Thomas

Thank you for your comments.

On Thu, 11 Jun 2009 05:31:49 -0400
Thomas Graf <tgraf@infradead.org> wrote:

> Very nice, this is a big improvement.
> 
> On Thu, Jun 11, 2009 at 06:05:55PM +0900, Minoru Usui wrote:
> > +static int get_tc_classid(u32 *classid, const char *str)
> 
> Maybe make this return u32?

Yes, you are right.
I'll fix it.

> 
> >  {
> > -	return cgrp_cls_state(cgrp)->classid;
> > +	u32 maj, min;
> > +	char *p;
> > +
> > +	/* check "0" for reset request */
> > +	if (!strcmp(str, "0")) {
> > +		*classid = 0;
> > +		return 0;
> > +	}
> > +
> > +	/* parse major number */
> > +	maj = simple_strtoul(str, &p, 16);
> > +	if (p == str) {
> > +		maj = 0;
> > +		if (*p != ':')
> > +			return -EINVAL;
> > +	}
> > +
> > +	/* parse minor number */
> > +	if (*p == ':') {
> > +		if (maj >= (1<<16))
> > +			return -EINVAL;
> > +
> > +		str = p + 1;
> > +		min = simple_strtoul(str, &p, 16);
> > +		if (*p != 0)
> > +			return -EINVAL;
> > +		if (min >= (1<<16))
> > +			return -EINVAL;
> > +	} else if (*p != 0)
> > +		return -EINVAL;
> 
> What do you think about keeping things backwards compatible by
> accepting both the X:Y and XY format?
> 
> } else if (*p != 0)
> 	return strtoul(str, NULL, 0);

OK. I'll do it.
-- 
Minoru Usui <usui@mxm.nes.nec.co.jp>