Patchwork [1/2] qemu-io: retry fgets() when errno is EINTR

login
register
mail settings
Submitter MORITA Kazutaka
Date June 15, 2010, 5:53 p.m.
Message ID <1276624421-23999-2-git-send-email-morita.kazutaka@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/55768/
State New
Headers show

Comments

MORITA Kazutaka - June 15, 2010, 5:53 p.m.
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(-)
Kevin Wolf - June 16, 2010, 11:04 a.m.
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
MORITA Kazutaka - June 16, 2010, 4:52 p.m.
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
Kevin Wolf - June 17, 2010, 8:27 a.m.
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
Jamie Lokier - June 17, 2010, 5:18 p.m.
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

Patch

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;
 	}