[PATCH] Some function locals in udev_rules_parse.c were needlessly

[PATCH] Some function locals in udev_rules_parse.c were needlessly

[Alan Jenkins]

This does not affect current behaviour.  However, it is required to
make the functions thread-safe.  (I'm playing with a threaded udevd).

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/udev/udev_rules_parse.c b/udev/udev_rules_parse.c
index 705b3fe..90b139b 100644
--- a/udev/udev_rules_parse.c
+++ b/udev/udev_rules_parse.c
@@ -41,7 +41,7 @@ void udev_rules_iter_init(struct udev_rules *rules)
 
 struct udev_rule *udev_rules_iter_next(struct udev_rules *rules)
 {
-	static struct udev_rule *rule;
+	struct udev_rule *rule;
 
 	if (!rules)
 		return NULL;
@@ -61,7 +61,7 @@ struct udev_rule *udev_rules_iter_next(struct udev_rules *rules)
 
 struct udev_rule *udev_rules_iter_label(struct udev_rules *rules, const char *label)
 {
-	static struct udev_rule *rule;
+	struct udev_rule *rule;
 	size_t start = rules->current;
 
 next:

Re: [PATCH] Some function locals in udev_rules_parse.c were needlessly declared as static

[Kay Sievers]

On Sun, Aug 31, 2008 at 19:30, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> This does not affect current behaviour.  However, it is required to
> make the functions thread-safe.  (I'm playing with a threaded udevd).

> -       static struct udev_rule *rule;
> +       struct udev_rule *rule;

It's not needlessly static, we return _this_ value. The parsing stuff
is not thread safe at the moment, we would need a real fix, this would
break it.

Thanks,
Kay

Re: [PATCH] Some function locals in udev_rules_parse.c were needlessly

[Alan Jenkins]

Kay Sievers wrote:
> On Sun, Aug 31, 2008 at 19:30, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>   
>> This does not affect current behaviour.  However, it is required to
>> make the functions thread-safe.  (I'm playing with a threaded udevd).
>>     
>
>   
>> -       static struct udev_rule *rule;
>> +       struct udev_rule *rule;
>>     
>
> It's not needlessly static, we return _this_ value. The parsing stuff
> is not thread safe at the moment, we would need a real fix, this would
> break it.
>   
Thanks for looking at these patches.

The functions do "return rule", but they always write to it before
reading it, so there's no persistent state here.  And they don't "return
&rule", so it's fine for the variable to be on the stack.  Did I miss
something?

I belatedly noticed the other parsing stuff a few hours after posting
the patch :-).  I hacked it up and (with unpublished patches) finally
got a threaded udevd which appeared to work.  (I also did per-thread
environment variable emulation, and fixed the caches in udev_sysfs.c for
thread-safety).  So empirically I had judged this patch correct.  And my
unpublished patches wouldn't conflict with or obsolete this one.

Regards
Alan

Re: [PATCH] Some function locals in udev_rules_parse.c were

[Kay Sievers]

On Mon, 2008-09-01 at 18:37 +0100, Alan Jenkins wrote:
> Kay Sievers wrote:
> > On Sun, Aug 31, 2008 at 19:30, Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
> >   
> >> This does not affect current behaviour.  However, it is required to
> >> make the functions thread-safe.  (I'm playing with a threaded udevd).
> >>     
> >
> >   
> >> -       static struct udev_rule *rule;
> >> +       struct udev_rule *rule;
> >>     
> >
> > It's not needlessly static, we return _this_ value. The parsing stuff
> > is not thread safe at the moment, we would need a real fix, this would
> > break it.
> >   
> Thanks for looking at these patches.
> 
> The functions do "return rule", but they always write to it before
> reading it, so there's no persistent state here.  And they don't "return
> &rule", so it's fine for the variable to be on the stack.  Did I miss
> something?

Ah, they should return a pointer to the data passed in to the function,
which should be fine.

Applied.

Thanks,
Kay