[PATCH] ftrace: remove check of list iterator against head past the loop body

[PATCH] ftrace: remove check of list iterator against head past the loop body

[Jakob Koschel]

When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.

While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
&pos->member == head, using the iterator variable after the loop should
be avoided.

In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].

Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
---
 kernel/trace/ftrace.c       | 20 ++++++++++++--------
 kernel/trace/trace_eprobe.c | 14 ++++++++------
 kernel/trace/trace_events.c | 12 ++++++------
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4f1d2f5e7263..096f5a83358d 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4560,8 +4560,8 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
 			       struct ftrace_probe_ops *probe_ops,
 			       void *data)
 {
+	struct ftrace_func_probe *probe = NULL, *iter;
 	struct ftrace_func_entry *entry;
-	struct ftrace_func_probe *probe;
 	struct ftrace_hash **orig_hash;
 	struct ftrace_hash *old_hash;
 	struct ftrace_hash *hash;
@@ -4580,11 +4580,13 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
 
 	mutex_lock(&ftrace_lock);
 	/* Check if the probe_ops is already registered */
-	list_for_each_entry(probe, &tr->func_probes, list) {
-		if (probe->probe_ops == probe_ops)
+	list_for_each_entry(iter, &tr->func_probes, list) {
+		if (iter->probe_ops == probe_ops) {
+			probe = iter;
 			break;
+		}
 	}
-	if (&probe->list == &tr->func_probes) {
+	if (!probe) {
 		probe = kzalloc(sizeof(*probe), GFP_KERNEL);
 		if (!probe) {
 			mutex_unlock(&ftrace_lock);
@@ -4704,7 +4706,7 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
 {
 	struct ftrace_ops_hash old_hash_ops;
 	struct ftrace_func_entry *entry;
-	struct ftrace_func_probe *probe;
+	struct ftrace_func_probe *probe = NULL, *iter;
 	struct ftrace_glob func_g;
 	struct ftrace_hash **orig_hash;
 	struct ftrace_hash *old_hash;
@@ -4732,11 +4734,13 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
 
 	mutex_lock(&ftrace_lock);
 	/* Check if the probe_ops is already registered */
-	list_for_each_entry(probe, &tr->func_probes, list) {
-		if (probe->probe_ops == probe_ops)
+	list_for_each_entry(iter, &tr->func_probes, list) {
+		if (iter->probe_ops == probe_ops) {
+			probe = iter;
 			break;
+		}
 	}
-	if (&probe->list == &tr->func_probes)
+	if (!probe)
 		goto err_unlock_ftrace;
 
 	ret = -EINVAL;
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 541aa13581b9..63e901a28425 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -650,7 +650,7 @@ static struct trace_event_functions eprobe_funcs = {
 static int disable_eprobe(struct trace_eprobe *ep,
 			  struct trace_array *tr)
 {
-	struct event_trigger_data *trigger;
+	struct event_trigger_data *trigger = NULL, *iter;
 	struct trace_event_file *file;
 	struct eprobe_data *edata;
 
@@ -658,14 +658,16 @@ static int disable_eprobe(struct trace_eprobe *ep,
 	if (!file)
 		return -ENOENT;
 
-	list_for_each_entry(trigger, &file->triggers, list) {
-		if (!(trigger->flags & EVENT_TRIGGER_FL_PROBE))
+	list_for_each_entry(iter, &file->triggers, list) {
+		if (!(iter->flags & EVENT_TRIGGER_FL_PROBE))
 			continue;
-		edata = trigger->private_data;
-		if (edata->ep == ep)
+		edata = iter->private_data;
+		if (edata->ep == ep) {
+			trigger = iter;
 			break;
+		}
 	}
-	if (list_entry_is_head(trigger, &file->triggers, list))
+	if (!trigger)
 		return -ENODEV;
 
 	list_del_rcu(&trigger->list);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index e11e167b7809..fe3dc157e635 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2281,7 +2281,7 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
 		    struct trace_event_file *file, struct dentry *parent)
 {
 	struct trace_subsystem_dir *dir;
-	struct event_subsystem *system;
+	struct event_subsystem *system, *iter;
 	struct dentry *entry;
 
 	/* First see if we did not already create this dir */
@@ -2295,13 +2295,13 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
 	}
 
 	/* Now see if the system itself exists. */
-	list_for_each_entry(system, &event_subsystems, list) {
-		if (strcmp(system->name, name) == 0)
+	system = NULL;
+	list_for_each_entry(iter, &event_subsystems, list) {
+		if (strcmp(iter->name, name) == 0) {
+			system = iter;
 			break;
+		}
 	}
-	/* Reset system variable when not found */
-	if (&system->list == &event_subsystems)
-		system = NULL;
 
 	dir = kmalloc(sizeof(*dir), GFP_KERNEL);
 	if (!dir)

base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275
-- 
2.25.1

Re: [PATCH] ftrace: remove check of list iterator against head past the loop body

[Steven Rostedt]

On Fri,  1 Apr 2022 00:37:52 +0200
Jakob Koschel <jakobkoschel@gmail.com> wrote:

Hi Jakob,

The patch looks fine, I have some small nits though.

First, the subject should actually be:

Subject: [PATCH] tracing: Remove check of list iterator against head past the loop body

Changes that only deal specifically with the function probes are labeled as
"ftrace:", but the more generic changes that touches files outside of
ftrace.c and fgraph.c should be "tracing:". Also, Linus prefers to have the
next part of the subject start with a capital letter: "Remove" instead of
"remove".

> When list_for_each_entry() completes the iteration over the whole list
> without breaking the loop, the iterator value will be a bogus pointer
> computed based on the head element.
> 
> While it is safe to use the pointer to determine if it was computed
> based on the head element, either with list_entry_is_head() or
> &pos->member == head, using the iterator variable after the loop should
> be avoided.
> 
> In preparation to limit the scope of a list iterator to the list
> traversal loop, use a dedicated pointer to point to the found element [1].
> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>  kernel/trace/ftrace.c       | 20 ++++++++++++--------
>  kernel/trace/trace_eprobe.c | 14 ++++++++------
>  kernel/trace/trace_events.c | 12 ++++++------
>  3 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4f1d2f5e7263..096f5a83358d 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -4560,8 +4560,8 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
>  			       struct ftrace_probe_ops *probe_ops,
>  			       void *data)
>  {
> +	struct ftrace_func_probe *probe = NULL, *iter;
>  	struct ftrace_func_entry *entry;
> -	struct ftrace_func_probe *probe;
>  	struct ftrace_hash **orig_hash;
>  	struct ftrace_hash *old_hash;
>  	struct ftrace_hash *hash;
> @@ -4580,11 +4580,13 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
>  
>  	mutex_lock(&ftrace_lock);
>  	/* Check if the probe_ops is already registered */
> -	list_for_each_entry(probe, &tr->func_probes, list) {
> -		if (probe->probe_ops == probe_ops)
> +	list_for_each_entry(iter, &tr->func_probes, list) {
> +		if (iter->probe_ops == probe_ops) {
> +			probe = iter;
>  			break;
> +		}
>  	}
> -	if (&probe->list == &tr->func_probes) {
> +	if (!probe) {
>  		probe = kzalloc(sizeof(*probe), GFP_KERNEL);
>  		if (!probe) {
>  			mutex_unlock(&ftrace_lock);
> @@ -4704,7 +4706,7 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
>  {
>  	struct ftrace_ops_hash old_hash_ops;
>  	struct ftrace_func_entry *entry;
> -	struct ftrace_func_probe *probe;
> +	struct ftrace_func_probe *probe = NULL, *iter;

Can you move this to the first declaration to keep the nice "upside-down
x-mas tree" look.

>  	struct ftrace_glob func_g;
>  	struct ftrace_hash **orig_hash;
>  	struct ftrace_hash *old_hash;
> @@ -4732,11 +4734,13 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
>  
>  	mutex_lock(&ftrace_lock);
>  	/* Check if the probe_ops is already registered */
> -	list_for_each_entry(probe, &tr->func_probes, list) {
> -		if (probe->probe_ops == probe_ops)
> +	list_for_each_entry(iter, &tr->func_probes, list) {
> +		if (iter->probe_ops == probe_ops) {
> +			probe = iter;
>  			break;
> +		}
>  	}
> -	if (&probe->list == &tr->func_probes)
> +	if (!probe)
>  		goto err_unlock_ftrace;
>  
>  	ret = -EINVAL;
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 541aa13581b9..63e901a28425 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -650,7 +650,7 @@ static struct trace_event_functions eprobe_funcs = {
>  static int disable_eprobe(struct trace_eprobe *ep,
>  			  struct trace_array *tr)
>  {
> -	struct event_trigger_data *trigger;
> +	struct event_trigger_data *trigger = NULL, *iter;
>  	struct trace_event_file *file;
>  	struct eprobe_data *edata;
>  
> @@ -658,14 +658,16 @@ static int disable_eprobe(struct trace_eprobe *ep,
>  	if (!file)
>  		return -ENOENT;
>  
> -	list_for_each_entry(trigger, &file->triggers, list) {
> -		if (!(trigger->flags & EVENT_TRIGGER_FL_PROBE))
> +	list_for_each_entry(iter, &file->triggers, list) {
> +		if (!(iter->flags & EVENT_TRIGGER_FL_PROBE))
>  			continue;
> -		edata = trigger->private_data;
> -		if (edata->ep == ep)
> +		edata = iter->private_data;
> +		if (edata->ep == ep) {
> +			trigger = iter;
>  			break;
> +		}
>  	}
> -	if (list_entry_is_head(trigger, &file->triggers, list))
> +	if (!trigger)
>  		return -ENODEV;
>  
>  	list_del_rcu(&trigger->list);
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index e11e167b7809..fe3dc157e635 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2281,7 +2281,7 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
>  		    struct trace_event_file *file, struct dentry *parent)
>  {
>  	struct trace_subsystem_dir *dir;
> -	struct event_subsystem *system;
> +	struct event_subsystem *system, *iter;

And move this above dir as well, for the same reason.

Thanks,

-- Steve


>  	struct dentry *entry;
>  
>  	/* First see if we did not already create this dir */
> @@ -2295,13 +2295,13 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
>  	}
>  
>  	/* Now see if the system itself exists. */
> -	list_for_each_entry(system, &event_subsystems, list) {
> -		if (strcmp(system->name, name) == 0)
> +	system = NULL;
> +	list_for_each_entry(iter, &event_subsystems, list) {
> +		if (strcmp(iter->name, name) == 0) {
> +			system = iter;
>  			break;
> +		}
>  	}
> -	/* Reset system variable when not found */
> -	if (&system->list == &event_subsystems)
> -		system = NULL;
>  
>  	dir = kmalloc(sizeof(*dir), GFP_KERNEL);
>  	if (!dir)
> 
> base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275

Re: [PATCH] ftrace: remove check of list iterator against head past the loop body

[Jakob Koschel]

> On 1. Apr 2022, at 01:10, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Fri, 1 Apr 2022 00:37:52 +0200
> Jakob Koschel <jakobkoschel@gmail.com> wrote:
> 
> Hi Jakob,
> 
> The patch looks fine, I have some small nits though.
> 
> First, the subject should actually be:
> 
> Subject: [PATCH] tracing: Remove check of list iterator against head past the loop body
> 
> Changes that only deal specifically with the function probes are labeled as
> "ftrace:", but the more generic changes that touches files outside of
> ftrace.c and fgraph.c should be "tracing:". Also, Linus prefers to have the
> next part of the subject start with a capital letter: "Remove" instead of
> "remove".

Thanks for the input. I've just seen that I have a few more changes that
I classified as 'tracing: *' so I'll bundle these up and resent altogether.

Also thanks for the advice on using a capital letter, I'll incorporate that.

> 
>> When list_for_each_entry() completes the iteration over the whole list
>> without breaking the loop, the iterator value will be a bogus pointer
>> computed based on the head element.
>> 
>> While it is safe to use the pointer to determine if it was computed
>> based on the head element, either with list_entry_is_head() or
>> &pos->member == head, using the iterator variable after the loop should
>> be avoided.
>> 
>> In preparation to limit the scope of a list iterator to the list
>> traversal loop, use a dedicated pointer to point to the found element [1].
>> 
>> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
>> ---
>> kernel/trace/ftrace.c | 20 ++++++++++++--------
>> kernel/trace/trace_eprobe.c | 14 ++++++++------
>> kernel/trace/trace_events.c | 12 ++++++------
>> 3 files changed, 26 insertions(+), 20 deletions(-)
>> 
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index 4f1d2f5e7263..096f5a83358d 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -4560,8 +4560,8 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
>> 			 struct ftrace_probe_ops *probe_ops,
>> 			 void *data)
>> {
>> +	struct ftrace_func_probe *probe = NULL, *iter;
>> 	struct ftrace_func_entry *entry;
>> -	struct ftrace_func_probe *probe;
>> 	struct ftrace_hash **orig_hash;
>> 	struct ftrace_hash *old_hash;
>> 	struct ftrace_hash *hash;
>> @@ -4580,11 +4580,13 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
>> 
>> 	mutex_lock(&ftrace_lock);
>> 	/* Check if the probe_ops is already registered */
>> -	list_for_each_entry(probe, &tr->func_probes, list) {
>> -		if (probe->probe_ops == probe_ops)
>> +	list_for_each_entry(iter, &tr->func_probes, list) {
>> +		if (iter->probe_ops == probe_ops) {
>> +			probe = iter;
>> 			break;
>> +		}
>> 	}
>> -	if (&probe->list == &tr->func_probes) {
>> +	if (!probe) {
>> 		probe = kzalloc(sizeof(*probe), GFP_KERNEL);
>> 		if (!probe) {
>> 			mutex_unlock(&ftrace_lock);
>> @@ -4704,7 +4706,7 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
>> {
>> 	struct ftrace_ops_hash old_hash_ops;
>> 	struct ftrace_func_entry *entry;
>> -	struct ftrace_func_probe *probe;
>> +	struct ftrace_func_probe *probe = NULL, *iter;
> 
> Can you move this to the first declaration to keep the nice "upside-down
> x-mas tree" look.

Thanks, I'll fix that up. It seems like this is not applied on the entire kernel
making treewide changes a bit more difficult. Is it documented somewhere, which
parts of the kernel enforce this? Just looking two lines down from here it
seems to be 'broken' already so just from looking at existing code it's often
hard to judge.

> 
>> 	struct ftrace_glob func_g;
>> 	struct ftrace_hash **orig_hash;
>> 	struct ftrace_hash *old_hash;
>> @@ -4732,11 +4734,13 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
>> 
>> 	mutex_lock(&ftrace_lock);
>> 	/* Check if the probe_ops is already registered */
>> -	list_for_each_entry(probe, &tr->func_probes, list) {
>> -		if (probe->probe_ops == probe_ops)
>> +	list_for_each_entry(iter, &tr->func_probes, list) {
>> +		if (iter->probe_ops == probe_ops) {
>> +			probe = iter;
>> 			break;
>> +		}
>> 	}
>> -	if (&probe->list == &tr->func_probes)
>> +	if (!probe)
>> 		goto err_unlock_ftrace;
>> 
>> 	ret = -EINVAL;
>> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
>> index 541aa13581b9..63e901a28425 100644
>> --- a/kernel/trace/trace_eprobe.c
>> +++ b/kernel/trace/trace_eprobe.c
>> @@ -650,7 +650,7 @@ static struct trace_event_functions eprobe_funcs = {
>> static int disable_eprobe(struct trace_eprobe *ep,
>> 			 struct trace_array *tr)
>> {
>> -	struct event_trigger_data *trigger;
>> +	struct event_trigger_data *trigger = NULL, *iter;
>> 	struct trace_event_file *file;
>> 	struct eprobe_data *edata;
>> 
>> @@ -658,14 +658,16 @@ static int disable_eprobe(struct trace_eprobe *ep,
>> 	if (!file)
>> 		return -ENOENT;
>> 
>> -	list_for_each_entry(trigger, &file->triggers, list) {
>> -		if (!(trigger->flags & EVENT_TRIGGER_FL_PROBE))
>> +	list_for_each_entry(iter, &file->triggers, list) {
>> +		if (!(iter->flags & EVENT_TRIGGER_FL_PROBE))
>> 			continue;
>> -		edata = trigger->private_data;
>> -		if (edata->ep == ep)
>> +		edata = iter->private_data;
>> +		if (edata->ep == ep) {
>> +			trigger = iter;
>> 			break;
>> +		}
>> 	}
>> -	if (list_entry_is_head(trigger, &file->triggers, list))
>> +	if (!trigger)
>> 		return -ENODEV;
>> 
>> 	list_del_rcu(&trigger->list);
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index e11e167b7809..fe3dc157e635 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -2281,7 +2281,7 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
>> 		 struct trace_event_file *file, struct dentry *parent)
>> {
>> 	struct trace_subsystem_dir *dir;
>> -	struct event_subsystem *system;
>> +	struct event_subsystem *system, *iter;
> 
> And move this above dir as well, for the same reason.
> 
> Thanks,
> 
> -- Steve
> 
> 
>> 	struct dentry *entry;
>> 
>> 	/* First see if we did not already create this dir */
>> @@ -2295,13 +2295,13 @@ event_subsystem_dir(struct trace_array *tr, const char *name,
>> 	}
>> 
>> 	/* Now see if the system itself exists. */
>> -	list_for_each_entry(system, &event_subsystems, list) {
>> -		if (strcmp(system->name, name) == 0)
>> +	system = NULL;
>> +	list_for_each_entry(iter, &event_subsystems, list) {
>> +		if (strcmp(iter->name, name) == 0) {
>> +			system = iter;
>> 			break;
>> +		}
>> 	}
>> -	/* Reset system variable when not found */
>> -	if (&system->list == &event_subsystems)
>> -		system = NULL;
>> 
>> 	dir = kmalloc(sizeof(*dir), GFP_KERNEL);
>> 	if (!dir)
>> 
>> base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275


	Jakob

Re: [PATCH] ftrace: remove check of list iterator against head past the loop body

[Steven Rostedt]

On Fri, 1 Apr 2022 01:22:58 +0200
Jakob Koschel <jakobkoschel@gmail.com> wrote:

> > Can you move this to the first declaration to keep the nice "upside-down
> > x-mas tree" look.  
> 
> Thanks, I'll fix that up. It seems like this is not applied on the entire kernel
> making treewide changes a bit more difficult. Is it documented somewhere, which
> parts of the kernel enforce this? Just looking two lines down from here it
> seems to be 'broken' already so just from looking at existing code it's often
> hard to judge.

It's one of those things that some maintainers prefer (I'm one of them ;-)
because it makes it easier to read IMHO.

But as you noticed, it's broken even in the same file. That's because I
don't strictly enforce it. If there's a lot of code that looks good to go
in, I don't ask to fix it. But as this was a small trivial patch, I figured
I'd mention it.

Thus, it's something that you do when asked, but don't worry about doing it
across the board, you are not going to upset anybody by forgetting to do it.

-- Steve

Re: [PATCH] ftrace: remove check of list iterator against head past the loop body

[Jakob Koschel]

> On 1. Apr 2022, at 01:34, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Fri, 1 Apr 2022 01:22:58 +0200
> Jakob Koschel <jakobkoschel@gmail.com> wrote:
> 
>>> Can you move this to the first declaration to keep the nice "upside-down
>>> x-mas tree" look.  
>> 
>> Thanks, I'll fix that up. It seems like this is not applied on the entire kernel
>> making treewide changes a bit more difficult. Is it documented somewhere, which
>> parts of the kernel enforce this? Just looking two lines down from here it
>> seems to be 'broken' already so just from looking at existing code it's often
>> hard to judge.
> 
> It's one of those things that some maintainers prefer (I'm one of them ;-)
> because it makes it easier to read IMHO.
> 
> But as you noticed, it's broken even in the same file. That's because I
> don't strictly enforce it. If there's a lot of code that looks good to go
> in, I don't ask to fix it. But as this was a small trivial patch, I figured
> I'd mention it.

I'm happy to fix it, I was just checking coding-style.rst and checkpatch.pl
and was hoping to find this documented somewhere.

> 
> Thus, it's something that you do when asked, but don't worry about doing it
> across the board, you are not going to upset anybody by forgetting to do it.

I guess then I'll stick with the strategy to incorporate when it's obvious
or to my knowledge (net/*) and otherwise fix when pointed out :)

Thanks for the additional info.

> 
> -- Steve

	Jakob