patch: namsiz

patch: namsiz

[Jamal Hadi Salim]

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


heres one ive wanted to do for a while - dissociate action name size
from ifnamsiz. for now set it to namize, but later may change it.

cheers,
jamal

[-- Attachment #2: bl_p --]
[-- Type: text/plain, Size: 1153 bytes --]

--- a/include/net/act_api.h	2005/01/16 13:23:00	1.1
+++ b/include/net/act_api.h	2005/01/16 13:24:13
@@ -40,6 +40,7 @@
 
 #define ACT_P_CREATED 1
 #define ACT_P_DELETED 1
+#define ANAMSIZ IFNAMSIZ
 
 struct tcf_act_hdr
 {
--- a/net/sched/act_api.c	2005/01/16 13:21:21	1.1
+++ b/net/sched/act_api.c	2005/01/16 13:24:33
@@ -227,7 +227,7 @@
 	if (a->ops == NULL || a->ops->dump == NULL)
 		return err;
 
-	RTA_PUT(skb, TCA_KIND, IFNAMSIZ, a->ops->kind);
+	RTA_PUT(skb, TCA_KIND, ANAMSIZ, a->ops->kind);
 	if (tcf_action_copy_stats(skb, a))
 		goto rtattr_failure;
 	r = (struct rtattr*) skb->tail;
@@ -272,7 +272,7 @@
 {
 	struct tc_action *a;
 	struct tc_action_ops *a_o;
-	char act_name[IFNAMSIZ];
+	char act_name[ANAMSIZ];
 	struct rtattr *tb[TCA_ACT_MAX+1];
 	struct rtattr *kind;
 
@@ -284,10 +284,10 @@
 		kind = tb[TCA_ACT_KIND-1];
 		if (kind == NULL)
 			goto err_out;
-		if (rtattr_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ)
+		if (rtattr_strlcpy(act_name, kind, ANAMSIZ) >= ANAMSIZ)
 			goto err_out;
 	} else {
-		if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ)
+		if (strlcpy(act_name, name, ANAMSIZ) >= ANAMSIZ)
 			goto err_out;
 	}
 

Re: patch: namsiz

[Patrick McHardy]

Jamal Hadi Salim wrote:

>heres one ive wanted to do for a while - dissociate action name size
>from ifnamsiz. for now set it to namize, but later may change it.
>
The same can be done for sch_api and cls_api. I'll add it to my tree
when I'm done with the current fixes, unfortunately one of them has
to touch basically all files in net/sched and I want to avoid clashes.

Regards
Patrick

Re: patch: namsiz

[Thomas Graf]

* Patrick McHardy <41EAB74F.6060507@trash.net> 2005-01-16 19:49
> Jamal Hadi Salim wrote:
> 
> >heres one ive wanted to do for a while - dissociate action name size
> >from ifnamsiz. for now set it to namize, but later may change it.
> >
> The same can be done for sch_api and cls_api. I'll add it to my tree
> when I'm done with the current fixes, unfortunately one of them has
> to touch basically all files in net/sched and I want to avoid clashes.

Go ahead, I have no changes in my tree except in a few header files
and the necessary iproute2 changes will occupy me for a while.

Re: patch: namsiz

[Jamal Hadi Salim]

On my part - theres one little change i made - just compiling it as we
speak;
I will post and stop there until you are done

cheers,
jamal

On Sun, 2005-01-16 at 13:56, Thomas Graf wrote:
> * Patrick McHardy <41EAB74F.6060507@trash.net> 2005-01-16 19:49
> > Jamal Hadi Salim wrote:
> > 
> > >heres one ive wanted to do for a while - dissociate action name size
> > >from ifnamsiz. for now set it to namize, but later may change it.
> > >
> > The same can be done for sch_api and cls_api. I'll add it to my tree
> > when I'm done with the current fixes, unfortunately one of them has
> > to touch basically all files in net/sched and I want to avoid clashes.
> 
> Go ahead, I have no changes in my tree except in a few header files
> and the necessary iproute2 changes will occupy me for a while.
> 
> 

path: module replay

[jamal]

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


Sorry, that work address just decides to appear when it feels like it.
Doing this from my laptop which is also used for work (as well as extra
curicular activities ;->). Now if evolution could setup the From address
for me when posting to netdev it would help ;->

On Sun, 2005-01-16 at 14:17, Jamal Hadi Salim wrote:
> On my part - theres one little change i made - just compiling it as we
> speak;
> I will post and stop there until you are done


Ok, here is the last patch. I think module replay should be done from
that spot and to be consistent as well from cls_api.c for legacy stuff.
I also fixed a module ref count leak. 
the act_api piece is dependent on what i sent earlier for namsiz.
the cls_api change i believe conflicts with what Thomas sent yesterday.
Anyways, not sending anymore after this ;->

cheers,
jamal

[-- Attachment #2: p_replay --]
[-- Type: text/plain, Size: 1680 bytes --]

--- a/net/sched/act_api.c	2005/01/16 14:05:37	1.2
+++ b/net/sched/act_api.c	2005/01/16 14:22:47
@@ -308,7 +308,7 @@
 		 */
 		if (a_o != NULL) {
 			*err = -EAGAIN;
-			goto err_mod;
+			goto err_out;
 		}
 #endif
 		goto err_out;
@@ -361,9 +361,13 @@
 	}
 
 	for (i=0; i < TCA_ACT_MAX_PRIO && tb[i]; i++) {
+replay:
 		act = tcf_action_init_1(tb[i], est, name, ovr, bind, err);
-		if (act == NULL)
-			goto err;
+		if (act == NULL) {
+			if (*err == -EAGAIN)
+				goto replay;
+			goto err_out;
+		}
 		act->order = i+1;
 
 		if (head == NULL)
@@ -374,7 +378,7 @@
 	}
 	return head;
 
-err:
+err_out:
 	if (head != NULL)
 		tcf_action_destroy(head, bind);
 	return NULL;
@@ -752,10 +756,7 @@
 		 */
 		if (n->nlmsg_flags&NLM_F_REPLACE)
 			ovr = 1;
-replay:
 		ret = tcf_action_add(tca[TCA_ACT_TAB-1], n, pid, ovr);
-		if (ret == -EAGAIN)
-			goto replay;
 		break;
 	case RTM_DELACTION:
 		ret = tca_action_gd(tca[TCA_ACT_TAB-1], n, pid, RTM_DELACTION);
--- a/net/sched/cls_api.c	2005/01/16 14:09:39	1.1
+++ b/net/sched/cls_api.c	2005/01/16 14:20:41
@@ -486,14 +486,19 @@
 	memset(exts, 0, sizeof(*exts));
 	
 #ifdef CONFIG_NET_CLS_ACT
+	{
 	int err;
 	struct tc_action *act;
 
 	if (map->police && tb[map->police-1]) {
+replay:
 		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
 			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
-		if (act == NULL)
-			return err;
+			if (act == NULL) {
+				if (err == -EAGAIN)
+					goto replay;
+				return err;
+			}
 
 		act->type = TCA_OLD_COMPAT;
 		exts->action = act;
@@ -505,6 +510,7 @@
 
 		exts->action = act;
 	}
+	}
 #elif defined CONFIG_NET_CLS_POLICE
 	if (map->police && tb[map->police-1]) {
 		struct tcf_police *p;

Re: path: module replay

[Patrick McHardy]

jamal wrote:

>Ok, here is the last patch. I think module replay should be done from
>that spot and to be consistent as well from cls_api.c for legacy stuff.
>I also fixed a module ref count leak. 
>the act_api piece is dependent on what i sent earlier for namsiz.
>the cls_api change i believe conflicts with what Thomas sent yesterday.
>
We have to replay all orders since we dropped the rtnl. An action
could have done __dev_get_by_* or something similar. Can you send
a single patch for the module refcount leak please ?

Regards
Patrick

Re: path: module replay

[Patrick McHardy]

Patrick McHardy wrote:

> jamal wrote:
>
>> Ok, here is the last patch. I think module replay should be done from
>> that spot and to be consistent as well from cls_api.c for legacy stuff.
>> I also fixed a module ref count leak. the act_api piece is dependent 
>> on what i sent earlier for namsiz.
>> the cls_api change i believe conflicts with what Thomas sent yesterday.
>>
> We have to replay all orders since we dropped the rtnl. An action
> could have done __dev_get_by_* or something similar. Can you send
> a single patch for the module refcount leak please ?

I was wrong, the already initialized actions need to hold their own
references anyway. I'm going have another look and add your patch later
if I don't see other problems.

Regards
Patrick

Re: path: module replay

[Patrick McHardy]

jamal wrote:

>Ok, here is the last patch. I think module replay should be done from
>that spot and to be consistent as well from cls_api.c for legacy stuff.
>I also fixed a module ref count leak. 
>the act_api piece is dependent on what i sent earlier for namsiz.
>the cls_api change i believe conflicts with what Thomas sent yesterday.
>
+replay:
                act = tcf_action_init_1(tb[i], est, name, ovr, bind, err);
-               if (act == NULL)
-                       goto err;
+               if (act == NULL) {
+                       if (*err == -EAGAIN)
+                               goto replay;
+                       goto err_out;
+               }

This part is wrong. We can replay single action requests in the act_api init
path, but not in tcf_exts_init. We are coming from a classifier 
initialization
path and have dropped the RTNL, so we also have to replay the classifier
request.

Please send a fixed patch without the bogus module refcnt change.

Regards
Patrick

Re: path: module replay

[jamal]

On Tue, 2005-01-18 at 23:26, Patrick McHardy wrote:
> jamal wrote:
> 
> >Ok, here is the last patch. I think module replay should be done from
> >that spot and to be consistent as well from cls_api.c for legacy stuff.
> >I also fixed a module ref count leak. 
> >the act_api piece is dependent on what i sent earlier for namsiz.
> >the cls_api change i believe conflicts with what Thomas sent yesterday.
> >
> +replay:
>                 act = tcf_action_init_1(tb[i], est, name, ovr, bind, err);
> -               if (act == NULL)
> -                       goto err;
> +               if (act == NULL) {
> +                       if (*err == -EAGAIN)
> +                               goto replay;
> +                       goto err_out;
> +               }
> 
> This part is wrong. 

And we exchanged emails on this topic already in private and you said 
you will fix and send out the patch;-> So please do just that or you can
wait until i have more time and my hardware at the same spot.

cheers,
jamal

Re: path: module replay

[Patrick McHardy]

jamal wrote:

>
>And we exchanged emails on this topic already in private and you said 
>you will fix and send out the patch;-> So please do just that or you can
>wait until i have more time and my hardware at the same spot.
>
I said I'm going to remove the bogus module loading changes and
apply the patch if there aren't more problems. But there are, and
my time is limited too, so I'm going to wait for a new patch.

Regards
Patrick

Re: path: module replay

[jamal]

On Tue, 2005-01-18 at 23:26, Patrick McHardy wrote:
> jamal wrote:
> 
> >Ok, here is the last patch. I think module replay should be done from
> >that spot and to be consistent as well from cls_api.c for legacy stuff.
> >I also fixed a module ref count leak. 
> >the act_api piece is dependent on what i sent earlier for namsiz.
> >the cls_api change i believe conflicts with what Thomas sent yesterday.
> >
> +replay:
>                 act = tcf_action_init_1(tb[i], est, name, ovr, bind, err);
> -               if (act == NULL)
> -                       goto err;
> +               if (act == NULL) {
> +                       if (*err == -EAGAIN)
> +                               goto replay;
> +                       goto err_out;
> +               }
> 
> This part is wrong. We can replay single action requests in the act_api init
> path, but not in tcf_exts_init. We are coming from a classifier 
> initialization
> path and have dropped the RTNL, so we also have to replay the classifier
> request.
> 
> Please send a fixed patch without the bogus module refcnt change.
> 

I just upgraded to rc2 + latest bk - compiling -  and looking at that
path again; i think what you have already is much cleaner. We may end up
getting a shorter code path for the non-classifier path in what i
suggested, but its not worth the cosmetic change. The one thing that
would be valuable to change is that goto i.e

---
-err:
+err_out:
---

since err is also a variable name. But i dont think this in itself
warrants a patch; maybe another patch that comes along later for
something else also takes care of this.

cheers,
jamal