[Spice-devel,usbredir,6/6] usbredirfilter: fix filter parsing for windows (mingw)

Submitted by Uri Lublin on May 3, 2012, 3:04 p.m.

Details

Message ID 1336057479-22629-7-git-send-email-uril@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin May 3, 2012, 3:04 p.m.
From: Arnon Gilboa <agilboa@redhat.com>

no strtok_r (reentrent) available, so use strtok instead.

Modified-by: Uri Lublin <uril@redhat.com>
---
 usbredirparser/usbredirfilter.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/usbredirparser/usbredirfilter.c b/usbredirparser/usbredirfilter.c
index b74c921..a1213cc 100644
--- a/usbredirparser/usbredirfilter.c
+++ b/usbredirparser/usbredirfilter.c
@@ -29,7 +29,7 @@  int usbredirfilter_string_to_rules(
     const char *filter_str, const char *token_sep, const char *rule_sep,
     struct usbredirfilter_rule **rules_ret, int *rules_count_ret)
 {
-    char *rule, *rule_saveptr, *token, *token_saveptr, *ep;
+    char *rule, *token, *ep, *buf_end, *rule_end;
     struct usbredirfilter_rule *rules = NULL;
     int i, rules_count, *values, ret = 0;
     char *buf = NULL;
@@ -63,17 +63,19 @@  int usbredirfilter_string_to_rules(
     }

     /* And actually parse the string */
+    buf_end = buf + strlen(buf);
     rules_count = 0;
-    rule = strtok_r(buf, rule_sep, &rule_saveptr);
+    rule = strtok(buf, rule_sep);
     while (rule) {
+        rule_end = rule + strlen(rule);
         /* We treat the filter rule as an array of ints for easier parsing */
         values = (int *)&rules[rules_count];
-        token = strtok_r(rule, token_sep, &token_saveptr);
+        token = strtok(rule, token_sep);
         for (i = 0; i < 5 && token; i++) {
             values[i] = strtol(token, &ep, 0);
             if (*ep)
                 break;
-            token = strtok_r(NULL, token_sep, &token_saveptr);
+            token = strtok(NULL, token_sep);
         }
         if (i != 5 || token != NULL ||
                 usbredirfilter_verify(&rules[rules_count], 1)) {
@@ -81,7 +83,11 @@  int usbredirfilter_string_to_rules(
             goto leave;
         }
         rules_count++;
-        rule = strtok_r(NULL, rule_sep, &rule_saveptr);
+        if (rule_end < buf_end) {
+            rule = strtok(rule_end + 1, rule_sep);
+        } else {
+            rule = NULL;
+        }
     }

     *rules_ret = rules;

Comments

On Thu, May 03, 2012 at 06:04:39PM +0300, Uri Lublin wrote:
> From: Arnon Gilboa <agilboa@redhat.com>
> 
> no strtok_r (reentrent) available, so use strtok instead.

Wouldn't it be safer to use strtok_s? Any application using threads and
strtok and libusbredirparser could get issues with this change no?

> 
> Modified-by: Uri Lublin <uril@redhat.com>
> ---
>  usbredirparser/usbredirfilter.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/usbredirparser/usbredirfilter.c b/usbredirparser/usbredirfilter.c
> index b74c921..a1213cc 100644
> --- a/usbredirparser/usbredirfilter.c
> +++ b/usbredirparser/usbredirfilter.c
> @@ -29,7 +29,7 @@ int usbredirfilter_string_to_rules(
>      const char *filter_str, const char *token_sep, const char *rule_sep,
>      struct usbredirfilter_rule **rules_ret, int *rules_count_ret)
>  {
> -    char *rule, *rule_saveptr, *token, *token_saveptr, *ep;
> +    char *rule, *token, *ep, *buf_end, *rule_end;
>      struct usbredirfilter_rule *rules = NULL;
>      int i, rules_count, *values, ret = 0;
>      char *buf = NULL;
> @@ -63,17 +63,19 @@ int usbredirfilter_string_to_rules(
>      }
> 
>      /* And actually parse the string */
> +    buf_end = buf + strlen(buf);
>      rules_count = 0;
> -    rule = strtok_r(buf, rule_sep, &rule_saveptr);
> +    rule = strtok(buf, rule_sep);
>      while (rule) {
> +        rule_end = rule + strlen(rule);
>          /* We treat the filter rule as an array of ints for easier parsing */
>          values = (int *)&rules[rules_count];
> -        token = strtok_r(rule, token_sep, &token_saveptr);
> +        token = strtok(rule, token_sep);
>          for (i = 0; i < 5 && token; i++) {
>              values[i] = strtol(token, &ep, 0);
>              if (*ep)
>                  break;
> -            token = strtok_r(NULL, token_sep, &token_saveptr);
> +            token = strtok(NULL, token_sep);
>          }
>          if (i != 5 || token != NULL ||
>                  usbredirfilter_verify(&rules[rules_count], 1)) {
> @@ -81,7 +83,11 @@ int usbredirfilter_string_to_rules(
>              goto leave;
>          }
>          rules_count++;
> -        rule = strtok_r(NULL, rule_sep, &rule_saveptr);
> +        if (rule_end < buf_end) {
> +            rule = strtok(rule_end + 1, rule_sep);
> +        } else {
> +            rule = NULL;
> +        }

rule = strtok(NULL, rule_sep); didn't work there? I'm bad with
strtok/strtok_r, but by reading the manpage, it doesn't seem things should
be handled differently there.

Christophe
Hi,

On 05/03/2012 06:09 PM, Christophe Fergeau wrote:
> On Thu, May 03, 2012 at 06:04:39PM +0300, Uri Lublin wrote:
>> From: Arnon Gilboa<agilboa@redhat.com>
>>
>> no strtok_r (reentrent) available, so use strtok instead.
>
> Wouldn't it be safer to use strtok_s? Any application using threads and
> strtok and libusbredirparser could get issues with this change no?

I have to agree with Christophe here, I would really like to keep
this code safe for calling from multiple threads, even if that
means some #ifdef stuff.

>> Modified-by: Uri Lublin<uril@redhat.com>
>> ---
>>   usbredirparser/usbredirfilter.c |   16 +++++++++++-----
>>   1 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/usbredirparser/usbredirfilter.c b/usbredirparser/usbredirfilter.c
>> index b74c921..a1213cc 100644
>> --- a/usbredirparser/usbredirfilter.c
>> +++ b/usbredirparser/usbredirfilter.c
>> @@ -29,7 +29,7 @@ int usbredirfilter_string_to_rules(
>>       const char *filter_str, const char *token_sep, const char *rule_sep,
>>       struct usbredirfilter_rule **rules_ret, int *rules_count_ret)
>>   {
>> -    char *rule, *rule_saveptr, *token, *token_saveptr, *ep;
>> +    char *rule, *token, *ep, *buf_end, *rule_end;
>>       struct usbredirfilter_rule *rules = NULL;
>>       int i, rules_count, *values, ret = 0;
>>       char *buf = NULL;
>> @@ -63,17 +63,19 @@ int usbredirfilter_string_to_rules(
>>       }
>>
>>       /* And actually parse the string */
>> +    buf_end = buf + strlen(buf);
>>       rules_count = 0;
>> -    rule = strtok_r(buf, rule_sep,&rule_saveptr);
>> +    rule = strtok(buf, rule_sep);
>>       while (rule) {
>> +        rule_end = rule + strlen(rule);
>>           /* We treat the filter rule as an array of ints for easier parsing */
>>           values = (int *)&rules[rules_count];
>> -        token = strtok_r(rule, token_sep,&token_saveptr);
>> +        token = strtok(rule, token_sep);
>>           for (i = 0; i<  5&&  token; i++) {
>>               values[i] = strtol(token,&ep, 0);
>>               if (*ep)
>>                   break;
>> -            token = strtok_r(NULL, token_sep,&token_saveptr);
>> +            token = strtok(NULL, token_sep);
>>           }
>>           if (i != 5 || token != NULL ||
>>                   usbredirfilter_verify(&rules[rules_count], 1)) {
>> @@ -81,7 +83,11 @@ int usbredirfilter_string_to_rules(
>>               goto leave;
>>           }
>>           rules_count++;
>> -        rule = strtok_r(NULL, rule_sep,&rule_saveptr);
>> +        if (rule_end<  buf_end) {
>> +            rule = strtok(rule_end + 1, rule_sep);
>> +        } else {
>> +            rule = NULL;
>> +        }
>
> rule = strtok(NULL, rule_sep); didn't work there? I'm bad with
> strtok/strtok_r, but by reading the manpage, it doesn't seem things should
> be handled differently there.

Again I agree with Christophe :)

All the other patches are ok. I don't know if you've noticed it yet,
but usbredir has an official git repo here now:
http://cgit.freedesktop.org/spice/usbredir/

Feel free to push the other patches there (since it is part of the
spice group git repos you should have commit access).

Regards,

Hans
On 05/03/2012 07:09 PM, Christophe Fergeau wrote:
> On Thu, May 03, 2012 at 06:04:39PM +0300, Uri Lublin wrote:
>> From: Arnon Gilboa<agilboa@redhat.com>
>>
>> no strtok_r (reentrent) available, so use strtok instead.
> Wouldn't it be safer to use strtok_s? Any application using threads and
> strtok and libusbredirparser could get issues with this change no?
>

It seems mingw32 does not support strtok_s

The first implementation was with ifdef for linux and Win32.
I'll send another patch with it.

Thanks for the quick review,
     Uri.
Hey,

On Mon, May 07, 2012 at 04:30:22PM +0300, Uri Lublin wrote:
> On 05/03/2012 07:09 PM, Christophe Fergeau wrote:
> >On Thu, May 03, 2012 at 06:04:39PM +0300, Uri Lublin wrote:
> >>From: Arnon Gilboa<agilboa@redhat.com>
> >>
> >>no strtok_r (reentrent) available, so use strtok instead.
> >Wouldn't it be safer to use strtok_s? Any application using threads and
> >strtok and libusbredirparser could get issues with this change no?
> >
> 
> It seems mingw32 does not support strtok_s

#include <string.h>

int main (int argc, char **argv)
{
    char *ctxt;
    char *token;
    token = strtok_s("foo", ":", &ctxt);

    return (token != NULL);
}

compiles and links fine on my f17 both with i686-w64-mingw32-gcc -Wall and
x86_64-w64-mingw32-gcc, so using strtok_s should be fine.

Christophe
On 05/09/2012 01:07 PM, Christophe Fergeau wrote:
> Hey,
>
> On Mon, May 07, 2012 at 04:30:22PM +0300, Uri Lublin wrote:
>> On 05/03/2012 07:09 PM, Christophe Fergeau wrote:
>>> On Thu, May 03, 2012 at 06:04:39PM +0300, Uri Lublin wrote:
>>>> From: Arnon Gilboa<agilboa@redhat.com>
>>>>
>>>> no strtok_r (reentrent) available, so use strtok instead.
>>> Wouldn't it be safer to use strtok_s? Any application using threads and
>>> strtok and libusbredirparser could get issues with this change no?
>>>
>> It seems mingw32 does not support strtok_s
> #include<string.h>
>
> int main (int argc, char **argv)
> {
>      char *ctxt;
>      char *token;
>      token = strtok_s("foo", ":",&ctxt);
>
>      return (token != NULL);
> }
>
> compiles and links fine on my f17 both with i686-w64-mingw32-gcc -Wall and
> x86_64-w64-mingw32-gcc, so using strtok_s should be fine.
>

On my f16 neither of those mingw gcc executables are available.

I'll send a simple patch using strtok_s for Windows

Thanks,
     Uri.