| Message ID | 1459843408-44840-1-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 5, 2016 at 3:03 AM, Christophe Milard < christophe.milard@linaro.org> wrote: > Fixes: https://bugs.linaro.org/show_bug.cgi?id=2146 (CID 159395) > The open system call is directely used to check the presence of the fifo > and open it at the same time. > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org> > --- > since v4: test after loop (Maxim) > since v3: nb_sec incremented in loop rather then at test time. (Maxim) > since v2: bug URL added (Anders) > since v1: changed loop to avoid open() line duplication (Maxim) > > platform/linux-generic/test/shmem/shmem_linux.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/platform/linux-generic/test/shmem/shmem_linux.c > b/platform/linux-generic/test/shmem/shmem_linux.c > index 12266cc..5a9be4d 100644 > --- a/platform/linux-generic/test/shmem/shmem_linux.c > +++ b/platform/linux-generic/test/shmem/shmem_linux.c > @@ -50,6 +50,7 @@ > > #define ODP_APP_NAME "shmem_odp" /* name of the odp program, in this dir > */ > #define DEVNAME_FMT "odp-%d-%s" /* shm device format: odp-<pid>-<name> > */ > +#define MAX_FIFO_WAIT 30 /* Max time waiting for the fifo (sec) > */ > > void test_success(char *fifo_name, int fd, pid_t odp_app) > { > @@ -89,7 +90,7 @@ int main(int argc __attribute__((unused)), char *argv[]) > { > char prg_name[PATH_MAX]; > char odp_name[PATH_MAX]; > - int nb_sec = 0; > + int nb_sec; > int size; > pid_t odp_app; > char *odp_params = NULL; > @@ -115,12 +116,14 @@ int main(int argc __attribute__((unused)), char > *argv[]) > * Just die if time expire as there is no fifo to communicate > * through... */ > sprintf(fifo_name, FIFO_NAME_FMT, odp_app); > - while (access(fifo_name, W_OK) != 0) { > + for (nb_sec = 0; nb_sec < MAX_FIFO_WAIT; nb_sec++) { > + fifo_fd = open(fifo_name, O_WRONLY); > + if (fifo_fd >= 0) > + break; > sleep(1); > - if (nb_sec++ == 30) > - exit(1); > } > - fifo_fd = open(fifo_name, O_WRONLY); > + if (nb_sec >= MAX_FIFO_WAIT) > + exit(1); > Shouldn't the real test here be: if (fifo_fd < 0) exit(1); That's the actual error condition you're checking for. nb_sec >= MAX_FIFO_WAIT is an indirect test based on the previous loop running to completion, however it's error prone if some future maintenance is done that would result in the loop exiting early. > printf("pipe found\n"); > > /* the linux named pipe has now been found, meaning that the > -- > 2.1.4 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
I agree. But this was required by Maxim, If I understood him right. You can have a look at the previous versions (v4, v3) for different alternatives. I don't know what to do now. Maxim & Bill: If you can agree on something, I'd be happy to round that up :-). Christophe On 6 April 2016 at 02:18, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > On Tue, Apr 5, 2016 at 3:03 AM, Christophe Milard < > christophe.milard@linaro.org> wrote: > >> Fixes: https://bugs.linaro.org/show_bug.cgi?id=2146 (CID 159395) >> The open system call is directely used to check the presence of the fifo >> and open it at the same time. >> >> Signed-off-by: Christophe Milard <christophe.milard@linaro.org> >> --- >> since v4: test after loop (Maxim) >> since v3: nb_sec incremented in loop rather then at test time. (Maxim) >> since v2: bug URL added (Anders) >> since v1: changed loop to avoid open() line duplication (Maxim) >> >> platform/linux-generic/test/shmem/shmem_linux.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/platform/linux-generic/test/shmem/shmem_linux.c >> b/platform/linux-generic/test/shmem/shmem_linux.c >> index 12266cc..5a9be4d 100644 >> --- a/platform/linux-generic/test/shmem/shmem_linux.c >> +++ b/platform/linux-generic/test/shmem/shmem_linux.c >> @@ -50,6 +50,7 @@ >> >> #define ODP_APP_NAME "shmem_odp" /* name of the odp program, in this dir >> */ >> #define DEVNAME_FMT "odp-%d-%s" /* shm device format: odp-<pid>-<name> >> */ >> +#define MAX_FIFO_WAIT 30 /* Max time waiting for the fifo (sec) >> */ >> >> void test_success(char *fifo_name, int fd, pid_t odp_app) >> { >> @@ -89,7 +90,7 @@ int main(int argc __attribute__((unused)), char *argv[]) >> { >> char prg_name[PATH_MAX]; >> char odp_name[PATH_MAX]; >> - int nb_sec = 0; >> + int nb_sec; >> int size; >> pid_t odp_app; >> char *odp_params = NULL; >> @@ -115,12 +116,14 @@ int main(int argc __attribute__((unused)), char >> *argv[]) >> * Just die if time expire as there is no fifo to communicate >> * through... */ >> sprintf(fifo_name, FIFO_NAME_FMT, odp_app); >> - while (access(fifo_name, W_OK) != 0) { >> + for (nb_sec = 0; nb_sec < MAX_FIFO_WAIT; nb_sec++) { >> + fifo_fd = open(fifo_name, O_WRONLY); >> + if (fifo_fd >= 0) >> + break; >> sleep(1); >> - if (nb_sec++ == 30) >> - exit(1); >> } >> - fifo_fd = open(fifo_name, O_WRONLY); >> + if (nb_sec >= MAX_FIFO_WAIT) >> + exit(1); >> > > Shouldn't the real test here be: > if (fifo_fd < 0) > exit(1); > > That's the actual error condition you're checking for. nb_sec >= > MAX_FIFO_WAIT is an indirect test based on the previous loop running to > completion, however it's error prone if some future maintenance is done > that would result in the loop exiting early. > > >> printf("pipe found\n"); >> >> /* the linux named pipe has now been found, meaning that the >> -- >> 2.1.4 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >
On 04/06/16 09:26, Christophe Milard wrote: > I agree. But this was required by Maxim, If I understood him right. > You can have a look at the previous versions (v4, v3) for different > alternatives. > I don't know what to do now. > Maxim & Bill: If you can agree on something, I'd be happy to round > that up :-). > > Christophe > I just looked previous patches and I think we are on the same way. use v5 and use Bills checks: change this: + if (nb_sec >= MAX_FIFO_WAIT) + exit(1); to: if (fifo_fd < 0) exit(1); because on the latest check (when nb_sec == MAX_FIFO_WAIT) open() can success. That might be needed to init fifo_fd to -1 to make checkpatch or Coverity happy. Maxim. > > > On 6 April 2016 at 02:18, Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> wrote: > > > > On Tue, Apr 5, 2016 at 3:03 AM, Christophe Milard > <christophe.milard@linaro.org > <mailto:christophe.milard@linaro.org>> wrote: > > Fixes: https://bugs.linaro.org/show_bug.cgi?id=2146 (CID 159395) > The open system call is directely used to check the presence > of the fifo > and open it at the same time. > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org > <mailto:christophe.milard@linaro.org>> > --- > since v4: test after loop (Maxim) > since v3: nb_sec incremented in loop rather then at test > time. (Maxim) > since v2: bug URL added (Anders) > since v1: changed loop to avoid open() line duplication (Maxim) > > platform/linux-generic/test/shmem/shmem_linux.c | 13 > ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/platform/linux-generic/test/shmem/shmem_linux.c > b/platform/linux-generic/test/shmem/shmem_linux.c > index 12266cc..5a9be4d 100644 > --- a/platform/linux-generic/test/shmem/shmem_linux.c > +++ b/platform/linux-generic/test/shmem/shmem_linux.c > @@ -50,6 +50,7 @@ > > #define ODP_APP_NAME "shmem_odp" /* name of the odp program, > in this dir */ > #define DEVNAME_FMT "odp-%d-%s" /* shm device format: > odp-<pid>-<name> */ > +#define MAX_FIFO_WAIT 30 /* Max time waiting for the > fifo (sec) */ > > void test_success(char *fifo_name, int fd, pid_t odp_app) > { > @@ -89,7 +90,7 @@ int main(int argc __attribute__((unused)), > char *argv[]) > { > char prg_name[PATH_MAX]; > char odp_name[PATH_MAX]; > - int nb_sec = 0; > + int nb_sec; > int size; > pid_t odp_app; > char *odp_params = NULL; > @@ -115,12 +116,14 @@ int main(int argc > __attribute__((unused)), char *argv[]) > * Just die if time expire as there is no fifo to > communicate > * through... */ > sprintf(fifo_name, FIFO_NAME_FMT, odp_app); > - while (access(fifo_name, W_OK) != 0) { > + for (nb_sec = 0; nb_sec < MAX_FIFO_WAIT; nb_sec++) { > + fifo_fd = open(fifo_name, O_WRONLY); > + if (fifo_fd >= 0) > + break; > sleep(1); > - if (nb_sec++ == 30) > - exit(1); > } > - fifo_fd = open(fifo_name, O_WRONLY); > + if (nb_sec >= MAX_FIFO_WAIT) > + exit(1); > > > Shouldn't the real test here be: > if (fifo_fd < 0) > exit(1); > > That's the actual error condition you're checking for. nb_sec >= > MAX_FIFO_WAIT is an indirect test based on the previous loop > running to completion, however it's error prone if some future > maintenance is done that would result in the loop exiting early. > > printf("pipe found\n"); > > /* the linux named pipe has now been found, meaning > that the > -- > 2.1.4 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > >
diff --git a/platform/linux-generic/test/shmem/shmem_linux.c b/platform/linux-generic/test/shmem/shmem_linux.c index 12266cc..5a9be4d 100644 --- a/platform/linux-generic/test/shmem/shmem_linux.c +++ b/platform/linux-generic/test/shmem/shmem_linux.c @@ -50,6 +50,7 @@ #define ODP_APP_NAME "shmem_odp" /* name of the odp program, in this dir */ #define DEVNAME_FMT "odp-%d-%s" /* shm device format: odp-<pid>-<name> */ +#define MAX_FIFO_WAIT 30 /* Max time waiting for the fifo (sec) */ void test_success(char *fifo_name, int fd, pid_t odp_app) { @@ -89,7 +90,7 @@ int main(int argc __attribute__((unused)), char *argv[]) { char prg_name[PATH_MAX]; char odp_name[PATH_MAX]; - int nb_sec = 0; + int nb_sec; int size; pid_t odp_app; char *odp_params = NULL; @@ -115,12 +116,14 @@ int main(int argc __attribute__((unused)), char *argv[]) * Just die if time expire as there is no fifo to communicate * through... */ sprintf(fifo_name, FIFO_NAME_FMT, odp_app); - while (access(fifo_name, W_OK) != 0) { + for (nb_sec = 0; nb_sec < MAX_FIFO_WAIT; nb_sec++) { + fifo_fd = open(fifo_name, O_WRONLY); + if (fifo_fd >= 0) + break; sleep(1); - if (nb_sec++ == 30) - exit(1); } - fifo_fd = open(fifo_name, O_WRONLY); + if (nb_sec >= MAX_FIFO_WAIT) + exit(1); printf("pipe found\n"); /* the linux named pipe has now been found, meaning that the
Fixes: https://bugs.linaro.org/show_bug.cgi?id=2146 (CID 159395) The open system call is directely used to check the presence of the fifo and open it at the same time. Signed-off-by: Christophe Milard <christophe.milard@linaro.org> --- since v4: test after loop (Maxim) since v3: nb_sec incremented in loop rather then at test time. (Maxim) since v2: bug URL added (Anders) since v1: changed loop to avoid open() line duplication (Maxim) platform/linux-generic/test/shmem/shmem_linux.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)