| Message ID | 1421059819-20423-1-git-send-email-ciprian.barbu@linaro.org |
|---|---|
| State | Accepted |
| Commit | 273955e6db6bb220f2736d3709e4237c50d04772 |
| Headers | show |
On 12 January 2015 at 11:50, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> > --- > Fix for https://bugs.linaro.org/show_bug.cgi?id=1049 > > example/packet/odp_pktio.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > index b162fac..0d5918a 100644 > --- a/example/packet/odp_pktio.c > +++ b/example/packet/odp_pktio.c > @@ -69,6 +69,7 @@ typedef struct { > int if_count; /**< Number of interfaces to be used */ > char **if_names; /**< Array of pointers to interface names */ > int mode; /**< Packet IO mode */ > + char *if_str; /**< Storage for interface names */ > } appl_args_t; > > /** > @@ -376,6 +377,8 @@ int main(int argc, char *argv[]) > /* Master thread waits for other threads to exit */ > odph_linux_pthread_join(thread_tbl, num_workers); > > + free(args->appl.if_names); > + free(args->appl.if_str); > free(args); > printf("Exit\n\n"); > > @@ -462,7 +465,7 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) > { > int opt; > int long_index; > - char *names, *str, *token, *save; > + char *token; > size_t len; > int i; > static struct option longopts[] = { > @@ -495,19 +498,19 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) > } > len += 1; /* add room for '\0' */ > > - names = malloc(len); > - if (names == NULL) { > + appl_args->if_str = malloc(len); > + if (appl_args->if_str == NULL) { > usage(argv[0]); > exit(EXIT_FAILURE); > } > > /* count the number of tokens separated by ',' */ > - strcpy(names, optarg); > - for (str = names, i = 0;; str = NULL, i++) { > - token = strtok_r(str, ",", &save); > - if (token == NULL) > - break; > - } > + strcpy(appl_args->if_str, optarg); > + for (token = strtok(appl_args->if_str, ","), i = 0; strtok() is not thread-safe and thus doesn't seem like a great idea to use it in a multithreaded application based on ODP. I don't know about this exact usage here, perhaps this argument parsing is always only going to happen in a single thread? But why not just define _POSIX_C_SOURCE to get access to strtok_r()? This is how we do it in other places in ODP with the same problem. E.g. the timer cunit test uses rand_r and not rand() (which likewise is not thread-safe). > + token != NULL; > + token = strtok(NULL, ","), i++) > + ; > + > appl_args->if_count = i; > > if (appl_args->if_count == 0) { > @@ -520,11 +523,9 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) > calloc(appl_args->if_count, sizeof(char *)); > > /* store the if names (reset names string) */ > - strcpy(names, optarg); > - for (str = names, i = 0;; str = NULL, i++) { > - token = strtok_r(str, ",", &save); > - if (token == NULL) > - break; > + strcpy(appl_args->if_str, optarg); > + for (token = strtok(appl_args->if_str, ","), i = 0; > + token != NULL; token = strtok(NULL, ","), i++) { > appl_args->if_names[i] = token; > } > break; > -- > 1.8.3.2 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
I have added _POSIX_C_SOURCE 200809L to one file already - patch on the list, will that version also cover this ? On 12 January 2015 at 07:22, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 12 January 2015 at 11:50, Ciprian Barbu <ciprian.barbu@linaro.org> > wrote: > > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> > > --- > > Fix for https://bugs.linaro.org/show_bug.cgi?id=1049 > > > > example/packet/odp_pktio.c | 29 +++++++++++++++-------------- > > 1 file changed, 15 insertions(+), 14 deletions(-) > > > > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > > index b162fac..0d5918a 100644 > > --- a/example/packet/odp_pktio.c > > +++ b/example/packet/odp_pktio.c > > @@ -69,6 +69,7 @@ typedef struct { > > int if_count; /**< Number of interfaces to be used */ > > char **if_names; /**< Array of pointers to interface > names */ > > int mode; /**< Packet IO mode */ > > + char *if_str; /**< Storage for interface names */ > > } appl_args_t; > > > > /** > > @@ -376,6 +377,8 @@ int main(int argc, char *argv[]) > > /* Master thread waits for other threads to exit */ > > odph_linux_pthread_join(thread_tbl, num_workers); > > > > + free(args->appl.if_names); > > + free(args->appl.if_str); > > free(args); > > printf("Exit\n\n"); > > > > @@ -462,7 +465,7 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > > { > > int opt; > > int long_index; > > - char *names, *str, *token, *save; > > + char *token; > > size_t len; > > int i; > > static struct option longopts[] = { > > @@ -495,19 +498,19 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > > } > > len += 1; /* add room for '\0' */ > > > > - names = malloc(len); > > - if (names == NULL) { > > + appl_args->if_str = malloc(len); > > + if (appl_args->if_str == NULL) { > > usage(argv[0]); > > exit(EXIT_FAILURE); > > } > > > > /* count the number of tokens separated by ',' */ > > - strcpy(names, optarg); > > - for (str = names, i = 0;; str = NULL, i++) { > > - token = strtok_r(str, ",", &save); > > - if (token == NULL) > > - break; > > - } > > + strcpy(appl_args->if_str, optarg); > > + for (token = strtok(appl_args->if_str, ","), i = > 0; > strtok() is not thread-safe and thus doesn't seem like a great idea to > use it in a multithreaded application based on ODP. I don't know about > this exact usage here, perhaps this argument parsing is always only > going to happen in a single thread? > > But why not just define _POSIX_C_SOURCE to get access to strtok_r()? > This is how we do it in other places in ODP with the same problem. > E.g. the timer cunit test uses rand_r and not rand() (which likewise > is not thread-safe). > > > > + token != NULL; > > + token = strtok(NULL, ","), i++) > > + ; > > + > > appl_args->if_count = i; > > > > if (appl_args->if_count == 0) { > > @@ -520,11 +523,9 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > > calloc(appl_args->if_count, sizeof(char *)); > > > > /* store the if names (reset names string) */ > > - strcpy(names, optarg); > > - for (str = names, i = 0;; str = NULL, i++) { > > - token = strtok_r(str, ",", &save); > > - if (token == NULL) > > - break; > > + strcpy(appl_args->if_str, optarg); > > + for (token = strtok(appl_args->if_str, ","), i = > 0; > > + token != NULL; token = strtok(NULL, ","), > i++) { > > appl_args->if_names[i] = token; > > } > > break; > > -- > > 1.8.3.2 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
I think so. The man page for strtok_r only states _POSIX_C_SOURC >= 1. On 12 January 2015 at 13:45, Mike Holmes <mike.holmes@linaro.org> wrote: > I have added _POSIX_C_SOURCE 200809L to one file already - patch on the > list, will that version also cover this ? > > On 12 January 2015 at 07:22, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> >> On 12 January 2015 at 11:50, Ciprian Barbu <ciprian.barbu@linaro.org> >> wrote: >> > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> >> > --- >> > Fix for https://bugs.linaro.org/show_bug.cgi?id=1049 >> > >> > example/packet/odp_pktio.c | 29 +++++++++++++++-------------- >> > 1 file changed, 15 insertions(+), 14 deletions(-) >> > >> > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c >> > index b162fac..0d5918a 100644 >> > --- a/example/packet/odp_pktio.c >> > +++ b/example/packet/odp_pktio.c >> > @@ -69,6 +69,7 @@ typedef struct { >> > int if_count; /**< Number of interfaces to be used */ >> > char **if_names; /**< Array of pointers to interface >> > names */ >> > int mode; /**< Packet IO mode */ >> > + char *if_str; /**< Storage for interface names */ >> > } appl_args_t; >> > >> > /** >> > @@ -376,6 +377,8 @@ int main(int argc, char *argv[]) >> > /* Master thread waits for other threads to exit */ >> > odph_linux_pthread_join(thread_tbl, num_workers); >> > >> > + free(args->appl.if_names); >> > + free(args->appl.if_str); >> > free(args); >> > printf("Exit\n\n"); >> > >> > @@ -462,7 +465,7 @@ static void parse_args(int argc, char *argv[], >> > appl_args_t *appl_args) >> > { >> > int opt; >> > int long_index; >> > - char *names, *str, *token, *save; >> > + char *token; >> > size_t len; >> > int i; >> > static struct option longopts[] = { >> > @@ -495,19 +498,19 @@ static void parse_args(int argc, char *argv[], >> > appl_args_t *appl_args) >> > } >> > len += 1; /* add room for '\0' */ >> > >> > - names = malloc(len); >> > - if (names == NULL) { >> > + appl_args->if_str = malloc(len); >> > + if (appl_args->if_str == NULL) { >> > usage(argv[0]); >> > exit(EXIT_FAILURE); >> > } >> > >> > /* count the number of tokens separated by ',' >> > */ >> > - strcpy(names, optarg); >> > - for (str = names, i = 0;; str = NULL, i++) { >> > - token = strtok_r(str, ",", &save); >> > - if (token == NULL) >> > - break; >> > - } >> > + strcpy(appl_args->if_str, optarg); >> > + for (token = strtok(appl_args->if_str, ","), i = >> > 0; >> strtok() is not thread-safe and thus doesn't seem like a great idea to >> use it in a multithreaded application based on ODP. I don't know about >> this exact usage here, perhaps this argument parsing is always only >> going to happen in a single thread? >> >> But why not just define _POSIX_C_SOURCE to get access to strtok_r()? >> This is how we do it in other places in ODP with the same problem. >> E.g. the timer cunit test uses rand_r and not rand() (which likewise >> is not thread-safe). >> >> >> > + token != NULL; >> > + token = strtok(NULL, ","), i++) >> > + ; >> > + >> > appl_args->if_count = i; >> > >> > if (appl_args->if_count == 0) { >> > @@ -520,11 +523,9 @@ static void parse_args(int argc, char *argv[], >> > appl_args_t *appl_args) >> > calloc(appl_args->if_count, sizeof(char *)); >> > >> > /* store the if names (reset names string) */ >> > - strcpy(names, optarg); >> > - for (str = names, i = 0;; str = NULL, i++) { >> > - token = strtok_r(str, ",", &save); >> > - if (token == NULL) >> > - break; >> > + strcpy(appl_args->if_str, optarg); >> > + for (token = strtok(appl_args->if_str, ","), i = >> > 0; >> > + token != NULL; token = strtok(NULL, ","), >> > i++) { >> > appl_args->if_names[i] = token; >> > } >> > break; >> > -- >> > 1.8.3.2 >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Linaro Sr Technical Manager > LNG - ODP
On 01/12/2015 03:22 PM, Ola Liljedahl wrote: > strtok() is not thread-safe and thus doesn't seem like a great idea to > use it in a multithreaded application based on ODP. I don't know about > this exact usage here, perhaps this argument parsing is always only > going to happen in a single thread? parse_args() done before any threads init. So it's safe here. If strtok is defined then it's better to go with it then define POSIX. +1 For Ciprians solution. Maxim.
Agree that if it is in single threaded section we keep the possibility of including more than we want out by not defining POSIX. On 12 January 2015 at 10:47, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 01/12/2015 03:22 PM, Ola Liljedahl wrote: > >> strtok() is not thread-safe and thus doesn't seem like a great idea to >> use it in a multithreaded application based on ODP. I don't know about >> this exact usage here, perhaps this argument parsing is always only >> going to happen in a single thread? >> > parse_args() done before any threads init. So it's safe here. If strtok is > defined then > it's better to go with it then define POSIX. > > +1 For Ciprians solution. > > Maxim. > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
From the man page "The *strtok*() function uses a static buffer while parsing, so it's not thread safe. Use *strtok_r*() if this matters to you." I tested it and it appears to work fine as patched and I don't think we need the arg parsing to be thread safe in this app as currently written. Mike On 12 January 2015 at 12:02, Mike Holmes <mike.holmes@linaro.org> wrote: > Agree that if it is in single threaded section we keep the possibility of > including more than we want out by not defining POSIX. > > > On 12 January 2015 at 10:47, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > >> On 01/12/2015 03:22 PM, Ola Liljedahl wrote: >> >>> strtok() is not thread-safe and thus doesn't seem like a great idea to >>> use it in a multithreaded application based on ODP. I don't know about >>> this exact usage here, perhaps this argument parsing is always only >>> going to happen in a single thread? >>> >> parse_args() done before any threads init. So it's safe here. If strtok >> is defined then >> it's better to go with it then define POSIX. >> >> +1 For Ciprians solution. >> >> Maxim. >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP >
On Tue, Jan 13, 2015 at 3:50 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > From the man page > > "The strtok() function uses a static buffer while parsing, so it's not > thread safe. Use strtok_r() if this matters to you." > > I tested it and it appears to work fine as patched and I don't think we need > the arg parsing to be thread safe in this app as currently written. It can be done either way, I only saw Mike's patch about _POSIX_C_SOURCE 200809L after I sent the patch. I also mistakenly thought the bug was assigned to me, that's why I worked on it, but it's still unclear what we are going to do with this bug. If we "fixed" stdc=c99 for good by defining POSIX source then we should clean all those bugs related to C99. > > > Mike > > On 12 January 2015 at 12:02, Mike Holmes <mike.holmes@linaro.org> wrote: >> >> Agree that if it is in single threaded section we keep the possibility of >> including more than we want out by not defining POSIX. >> >> >> On 12 January 2015 at 10:47, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> >>> On 01/12/2015 03:22 PM, Ola Liljedahl wrote: >>>> >>>> strtok() is not thread-safe and thus doesn't seem like a great idea to >>>> use it in a multithreaded application based on ODP. I don't know about >>>> this exact usage here, perhaps this argument parsing is always only >>>> going to happen in a single thread? >>> >>> parse_args() done before any threads init. So it's safe here. If strtok >>> is defined then >>> it's better to go with it then define POSIX. >>> >>> +1 For Ciprians solution. >>> >>> Maxim. >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- >> Mike Holmes >> Linaro Sr Technical Manager >> LNG - ODP > > > > > -- > Mike Holmes > Linaro Sr Technical Manager > LNG - ODP > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On Tue, Jan 13, 2015 at 12:18 PM, Stuart Haslam <stuart.haslam@arm.com> wrote: > On Tue, Jan 13, 2015 at 09:49:00AM +0000, Ciprian Barbu wrote: >> On Tue, Jan 13, 2015 at 3:50 AM, Mike Holmes <mike.holmes@linaro.org> wrote: >> > From the man page >> > >> > "The strtok() function uses a static buffer while parsing, so it's not >> > thread safe. Use strtok_r() if this matters to you." >> > >> > I tested it and it appears to work fine as patched and I don't think we need >> > the arg parsing to be thread safe in this app as currently written. >> >> It can be done either way, I only saw Mike's patch about >> _POSIX_C_SOURCE 200809L after I sent the patch. I also mistakenly >> thought the bug was assigned to me, that's why I worked on it, but >> it's still unclear what we are going to do with this bug. If we >> "fixed" stdc=c99 for good by defining POSIX source then we should >> clean all those bugs related to C99. >> > > Are there other open bugs for the l2fwd, generator and ipsec examples? > They contain a copy of the same interface name parsing code. > > Perhaps this code should be broken out into a shared file under > examples/common/. Yeah, that would be nice. But I don't have the cylces for that right now. > > -- > Stuart. >
On 13 January 2015 at 04:49, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > On Tue, Jan 13, 2015 at 3:50 AM, Mike Holmes <mike.holmes@linaro.org> > wrote: > > From the man page > > > > "The strtok() function uses a static buffer while parsing, so it's not > > thread safe. Use strtok_r() if this matters to you." > > > > I tested it and it appears to work fine as patched and I don't think we > need > > the arg parsing to be thread safe in this app as currently written. > > It can be done either way, I only saw Mike's patch about > _POSIX_C_SOURCE 200809L after I sent the patch. I also mistakenly > thought the bug was assigned to me, that's why I worked on it, but > it's still unclear what we are going to do with this bug. If we > "fixed" stdc=c99 for good by defining POSIX source then we should > clean all those bugs related to C99. > > We only apply _POSIX_C_SOURCE 200809L per source file if there is a function that really can't be done with C99 - ftruncate is the only one I am sure of so far. In this case we can stick with plain c99 without any potential issues so I think we should. > > > > > > Mike > > > > On 12 January 2015 at 12:02, Mike Holmes <mike.holmes@linaro.org> wrote: > >> > >> Agree that if it is in single threaded section we keep the possibility > of > >> including more than we want out by not defining POSIX. > >> > >> > >> On 12 January 2015 at 10:47, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > >>> > >>> On 01/12/2015 03:22 PM, Ola Liljedahl wrote: > >>>> > >>>> strtok() is not thread-safe and thus doesn't seem like a great idea to > >>>> use it in a multithreaded application based on ODP. I don't know about > >>>> this exact usage here, perhaps this argument parsing is always only > >>>> going to happen in a single thread? > >>> > >>> parse_args() done before any threads init. So it's safe here. If strtok > >>> is defined then > >>> it's better to go with it then define POSIX. > >>> > >>> +1 For Ciprians solution. > >>> > >>> Maxim. > >>> > >>> > >>> _______________________________________________ > >>> lng-odp mailing list > >>> lng-odp@lists.linaro.org > >>> http://lists.linaro.org/mailman/listinfo/lng-odp > >> > >> > >> > >> > >> -- > >> Mike Holmes > >> Linaro Sr Technical Manager > >> LNG - ODP > > > > > > > > > > -- > > Mike Holmes > > Linaro Sr Technical Manager > > LNG - ODP > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp > > >
On 12 January 2015 at 05:50, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> > Reviewed-and-tested-by: Mike Holmes <mike.holmes@linaro.org> > --- > Fix for https://bugs.linaro.org/show_bug.cgi?id=1049 > > example/packet/odp_pktio.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > index b162fac..0d5918a 100644 > --- a/example/packet/odp_pktio.c > +++ b/example/packet/odp_pktio.c > @@ -69,6 +69,7 @@ typedef struct { > int if_count; /**< Number of interfaces to be used */ > char **if_names; /**< Array of pointers to interface names > */ > int mode; /**< Packet IO mode */ > + char *if_str; /**< Storage for interface names */ > } appl_args_t; > > /** > @@ -376,6 +377,8 @@ int main(int argc, char *argv[]) > /* Master thread waits for other threads to exit */ > odph_linux_pthread_join(thread_tbl, num_workers); > > + free(args->appl.if_names); > + free(args->appl.if_str); > free(args); > printf("Exit\n\n"); > > @@ -462,7 +465,7 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > { > int opt; > int long_index; > - char *names, *str, *token, *save; > + char *token; > size_t len; > int i; > static struct option longopts[] = { > @@ -495,19 +498,19 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > } > len += 1; /* add room for '\0' */ > > - names = malloc(len); > - if (names == NULL) { > + appl_args->if_str = malloc(len); > + if (appl_args->if_str == NULL) { > usage(argv[0]); > exit(EXIT_FAILURE); > } > > /* count the number of tokens separated by ',' */ > - strcpy(names, optarg); > - for (str = names, i = 0;; str = NULL, i++) { > - token = strtok_r(str, ",", &save); > - if (token == NULL) > - break; > - } > + strcpy(appl_args->if_str, optarg); > + for (token = strtok(appl_args->if_str, ","), i = 0; > + token != NULL; > + token = strtok(NULL, ","), i++) > + ; > + > appl_args->if_count = i; > > if (appl_args->if_count == 0) { > @@ -520,11 +523,9 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > calloc(appl_args->if_count, sizeof(char *)); > > /* store the if names (reset names string) */ > - strcpy(names, optarg); > - for (str = names, i = 0;; str = NULL, i++) { > - token = strtok_r(str, ",", &save); > - if (token == NULL) > - break; > + strcpy(appl_args->if_str, optarg); > + for (token = strtok(appl_args->if_str, ","), i = 0; > + token != NULL; token = strtok(NULL, ","), > i++) { > appl_args->if_names[i] = token; > } > break; > -- > 1.8.3.2 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
Merged! Maxim. On 01/13/2015 09:52 PM, Mike Holmes wrote: > > > On 12 January 2015 at 05:50, Ciprian Barbu <ciprian.barbu@linaro.org > <mailto:ciprian.barbu@linaro.org>> wrote: > > Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org > <mailto:ciprian.barbu@linaro.org>> > > > Reviewed-and-tested-by: Mike Holmes <mike.holmes@linaro.org > <mailto:mike.holmes@linaro.org>> > > --- > Fix for https://bugs.linaro.org/show_bug.cgi?id=1049 > > example/packet/odp_pktio.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > index b162fac..0d5918a 100644 > --- a/example/packet/odp_pktio.c > +++ b/example/packet/odp_pktio.c > @@ -69,6 +69,7 @@ typedef struct { > int if_count; /**< Number of interfaces to be > used */ > char **if_names; /**< Array of pointers to > interface names */ > int mode; /**< Packet IO mode */ > + char *if_str; /**< Storage for interface names */ > } appl_args_t; > > /** > @@ -376,6 +377,8 @@ int main(int argc, char *argv[]) > /* Master thread waits for other threads to exit */ > odph_linux_pthread_join(thread_tbl, num_workers); > > + free(args->appl.if_names); > + free(args->appl.if_str); > free(args); > printf("Exit\n\n"); > > @@ -462,7 +465,7 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > { > int opt; > int long_index; > - char *names, *str, *token, *save; > + char *token; > size_t len; > int i; > static struct option longopts[] = { > @@ -495,19 +498,19 @@ static void parse_args(int argc, char > *argv[], appl_args_t *appl_args) > } > len += 1; /* add room for '\0' */ > > - names = malloc(len); > - if (names == NULL) { > + appl_args->if_str = malloc(len); > + if (appl_args->if_str == NULL) { > usage(argv[0]); > exit(EXIT_FAILURE); > } > > /* count the number of tokens separated by > ',' */ > - strcpy(names, optarg); > - for (str = names, i = 0;; str = NULL, i++) { > - token = strtok_r(str, ",", &save); > - if (token == NULL) > - break; > - } > + strcpy(appl_args->if_str, optarg); > + for (token = strtok(appl_args->if_str, > ","), i = 0; > + token != NULL; > + token = strtok(NULL, ","), i++) > + ; > + > appl_args->if_count = i; > > if (appl_args->if_count == 0) { > @@ -520,11 +523,9 @@ static void parse_args(int argc, char > *argv[], appl_args_t *appl_args) > calloc(appl_args->if_count, > sizeof(char *)); > > /* store the if names (reset names string) */ > - strcpy(names, optarg); > - for (str = names, i = 0;; str = NULL, i++) { > - token = strtok_r(str, ",", &save); > - if (token == NULL) > - break; > + strcpy(appl_args->if_str, optarg); > + for (token = strtok(appl_args->if_str, > ","), i = 0; > + token != NULL; token = strtok(NULL, > ","), i++) { > appl_args->if_names[i] = token; > } > break; > -- > 1.8.3.2 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 13 January 2015 at 05:18, Stuart Haslam <stuart.haslam@arm.com> wrote: > On Tue, Jan 13, 2015 at 09:49:00AM +0000, Ciprian Barbu wrote: > > On Tue, Jan 13, 2015 at 3:50 AM, Mike Holmes <mike.holmes@linaro.org> > wrote: > > > From the man page > > > > > > "The strtok() function uses a static buffer while parsing, so it's not > > > thread safe. Use strtok_r() if this matters to you." > > > > > > I tested it and it appears to work fine as patched and I don't think > we need > > > the arg parsing to be thread safe in this app as currently written. > > > > It can be done either way, I only saw Mike's patch about > > _POSIX_C_SOURCE 200809L after I sent the patch. I also mistakenly > > thought the bug was assigned to me, that's why I worked on it, but > > it's still unclear what we are going to do with this bug. If we > > "fixed" stdc=c99 for good by defining POSIX source then we should > > clean all those bugs related to C99. > > > > Are there other open bugs for the l2fwd, generator and ipsec examples? > They contain a copy of the same interface name parsing code. > > Perhaps this code should be broken out into a shared file under > examples/common/. > Tracked as https://bugs.linaro.org/show_bug.cgi?id=1079 > > -- > Stuart. > >
diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c index b162fac..0d5918a 100644 --- a/example/packet/odp_pktio.c +++ b/example/packet/odp_pktio.c @@ -69,6 +69,7 @@ typedef struct { int if_count; /**< Number of interfaces to be used */ char **if_names; /**< Array of pointers to interface names */ int mode; /**< Packet IO mode */ + char *if_str; /**< Storage for interface names */ } appl_args_t; /** @@ -376,6 +377,8 @@ int main(int argc, char *argv[]) /* Master thread waits for other threads to exit */ odph_linux_pthread_join(thread_tbl, num_workers); + free(args->appl.if_names); + free(args->appl.if_str); free(args); printf("Exit\n\n"); @@ -462,7 +465,7 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) { int opt; int long_index; - char *names, *str, *token, *save; + char *token; size_t len; int i; static struct option longopts[] = { @@ -495,19 +498,19 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) } len += 1; /* add room for '\0' */ - names = malloc(len); - if (names == NULL) { + appl_args->if_str = malloc(len); + if (appl_args->if_str == NULL) { usage(argv[0]); exit(EXIT_FAILURE); } /* count the number of tokens separated by ',' */ - strcpy(names, optarg); - for (str = names, i = 0;; str = NULL, i++) { - token = strtok_r(str, ",", &save); - if (token == NULL) - break; - } + strcpy(appl_args->if_str, optarg); + for (token = strtok(appl_args->if_str, ","), i = 0; + token != NULL; + token = strtok(NULL, ","), i++) + ; + appl_args->if_count = i; if (appl_args->if_count == 0) { @@ -520,11 +523,9 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) calloc(appl_args->if_count, sizeof(char *)); /* store the if names (reset names string) */ - strcpy(names, optarg); - for (str = names, i = 0;; str = NULL, i++) { - token = strtok_r(str, ",", &save); - if (token == NULL) - break; + strcpy(appl_args->if_str, optarg); + for (token = strtok(appl_args->if_str, ","), i = 0; + token != NULL; token = strtok(NULL, ","), i++) { appl_args->if_names[i] = token; } break;
Signed-off-by: Ciprian Barbu <ciprian.barbu@linaro.org> --- Fix for https://bugs.linaro.org/show_bug.cgi?id=1049 example/packet/odp_pktio.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)