| Message ID | 1276624421-23999-2-git-send-email-morita.kazutaka@lab.ntt.co.jp |
|---|---|
| State | New |
| Headers | show |
Am 15.06.2010 19:53, schrieb MORITA Kazutaka: > posix-aio-compat sends a signal in aio operations, so we should > consider that fgets() could be interrupted here. > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > --- > cmd.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/cmd.c b/cmd.c > index 2336334..460df92 100644 > --- a/cmd.c > +++ b/cmd.c > @@ -272,7 +272,10 @@ fetchline(void) > return NULL; > printf("%s", get_prompt()); > fflush(stdout); > +again: > if (!fgets(line, MAXREADLINESZ, stdin)) { > + if (errno == EINTR) > + goto again; > free(line); > return NULL; > } This looks like a loop replaced by goto (and braces are missing). What about this instead? do { ret = fgets(...) } while (ret == NULL && errno == EINTR) if (ret == NULL) { fail } Kevin
At Wed, 16 Jun 2010 13:04:47 +0200, Kevin Wolf wrote: > > Am 15.06.2010 19:53, schrieb MORITA Kazutaka: > > posix-aio-compat sends a signal in aio operations, so we should > > consider that fgets() could be interrupted here. > > > > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > > --- > > cmd.c | 3 +++ > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > diff --git a/cmd.c b/cmd.c > > index 2336334..460df92 100644 > > --- a/cmd.c > > +++ b/cmd.c > > @@ -272,7 +272,10 @@ fetchline(void) > > return NULL; > > printf("%s", get_prompt()); > > fflush(stdout); > > +again: > > if (!fgets(line, MAXREADLINESZ, stdin)) { > > + if (errno == EINTR) > > + goto again; > > free(line); > > return NULL; > > } > > This looks like a loop replaced by goto (and braces are missing). What > about this instead? > > do { > ret = fgets(...) > } while (ret == NULL && errno == EINTR) > > if (ret == NULL) { > fail > } > I agree. However, it seems that my second patch have already solved the problem. We register this readline routines as an aio handler now, so fgets() does not block and cannot return with EINTR. This patch looks no longer needed, sorry. Thanks, Kazutaka
Am 16.06.2010 18:52, schrieb MORITA Kazutaka: > At Wed, 16 Jun 2010 13:04:47 +0200, > Kevin Wolf wrote: >> >> Am 15.06.2010 19:53, schrieb MORITA Kazutaka: >>> posix-aio-compat sends a signal in aio operations, so we should >>> consider that fgets() could be interrupted here. >>> >>> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> >>> --- >>> cmd.c | 3 +++ >>> 1 files changed, 3 insertions(+), 0 deletions(-) >>> >>> diff --git a/cmd.c b/cmd.c >>> index 2336334..460df92 100644 >>> --- a/cmd.c >>> +++ b/cmd.c >>> @@ -272,7 +272,10 @@ fetchline(void) >>> return NULL; >>> printf("%s", get_prompt()); >>> fflush(stdout); >>> +again: >>> if (!fgets(line, MAXREADLINESZ, stdin)) { >>> + if (errno == EINTR) >>> + goto again; >>> free(line); >>> return NULL; >>> } >> >> This looks like a loop replaced by goto (and braces are missing). What >> about this instead? >> >> do { >> ret = fgets(...) >> } while (ret == NULL && errno == EINTR) >> >> if (ret == NULL) { >> fail >> } >> > > I agree. > > However, it seems that my second patch have already solved the > problem. We register this readline routines as an aio handler now, so > fgets() does not block and cannot return with EINTR. > > This patch looks no longer needed, sorry. Good point. Thanks for having a look. Kevin
Kevin Wolf wrote: > Am 16.06.2010 18:52, schrieb MORITA Kazutaka: > > At Wed, 16 Jun 2010 13:04:47 +0200, > > Kevin Wolf wrote: > >> > >> Am 15.06.2010 19:53, schrieb MORITA Kazutaka: > >>> posix-aio-compat sends a signal in aio operations, so we should > >>> consider that fgets() could be interrupted here. > >>> > >>> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> > >>> --- > >>> cmd.c | 3 +++ > >>> 1 files changed, 3 insertions(+), 0 deletions(-) > >>> > >>> diff --git a/cmd.c b/cmd.c > >>> index 2336334..460df92 100644 > >>> --- a/cmd.c > >>> +++ b/cmd.c > >>> @@ -272,7 +272,10 @@ fetchline(void) > >>> return NULL; > >>> printf("%s", get_prompt()); > >>> fflush(stdout); > >>> +again: > >>> if (!fgets(line, MAXREADLINESZ, stdin)) { > >>> + if (errno == EINTR) > >>> + goto again; > >>> free(line); > >>> return NULL; > >>> } > >> > >> This looks like a loop replaced by goto (and braces are missing). What > >> about this instead? > >> > >> do { > >> ret = fgets(...) > >> } while (ret == NULL && errno == EINTR) > >> > >> if (ret == NULL) { > >> fail > >> } > >> > > > > I agree. > > > > However, it seems that my second patch have already solved the > > problem. We register this readline routines as an aio handler now, so > > fgets() does not block and cannot return with EINTR. > > > > This patch looks no longer needed, sorry. > > Good point. Thanks for having a look. Anyway, are you sure stdio functions can be interrupted with EINTR? Linus reminds us that some stdio functions have to retry internally anyway: http://comments.gmane.org/gmane.comp.version-control.git/18285 -- Jamie
diff --git a/cmd.c b/cmd.c index 2336334..460df92 100644 --- a/cmd.c +++ b/cmd.c @@ -272,7 +272,10 @@ fetchline(void) return NULL; printf("%s", get_prompt()); fflush(stdout); +again: if (!fgets(line, MAXREADLINESZ, stdin)) { + if (errno == EINTR) + goto again; free(line); return NULL; }
posix-aio-compat sends a signal in aio operations, so we should consider that fgets() could be interrupted here. Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp> --- cmd.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)