Re: [PATCH 1/5] xfdopen(): if first attempt fails, free memory and try again
On Thu, Mar 5, 2015 at 8:07 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 31 quoted lines
> One likely reason for the failure of fdopen() is a lack of free
> memory.
>
> Also expose a new function, fdopen_with_retry(), which retries on
> ENOMEM but doesn't die() on errors. In a moment this function will be
> used elsewhere.
>
> Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> git-compat-util.h | 11 +++++++++++
> wrapper.c | 28 +++++++++++++++++++++++++---
> 2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3455c5e..a5652a7 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -672,7 +672,18 @@ extern ssize_t xread(int fd, void *buf, size_t len);
> extern ssize_t xwrite(int fd, const void *buf, size_t len);
> extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
> extern int xdup(int fd);
> +
> +/*
> + * Like fdopen(), but if the first attempt fails with ENOMEM, try to
> + * free up some memory and try again.
> + */
> +extern FILE *fdopen_with_retry(int fd, const char *mode);
> +
> +/*
> + * Like fdopen_with_retry(), but die on errors.
"Rather like fdopen, but die on errors"?
When reading this before looking at the implementation below,
I thought this would behave like
FILE *xfdopen() {
FILE *stream = fdopen(fd, mode);
if (stream == NULL)
die_errno("Out of memory? fdopen failed");
return stream;
}
which has no retrying. I kind of read that comment that way.
Maybe
This opens a FILE* handle, but tries
to deal with some errors. If unsuccessful it dies,
which guarantees to have a valid FILE* if returning.
?I dunno.
Show 45 quoted lines
> + */
> extern FILE *xfdopen(int fd, const char *mode);
> +
> extern int xmkstemp(char *template);
> extern int xmkstemp_mode(char *template, int mode);
> extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
> diff --git a/wrapper.c b/wrapper.c
> index d5a6cef..b60cc03 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -311,14 +311,36 @@ int xdup(int fd)
> return ret;
> }
>
> -FILE *xfdopen(int fd, const char *mode)
> +FILE *fdopen_with_retry(int fd, const char *mode)
> {
> FILE *stream = fdopen(fd, mode);
> - if (stream == NULL)
> - die_errno("Out of memory? fdopen failed");
> +
> + if (!stream && errno == ENOMEM) {
> + /*
> + * Try to free up some memory, then try again. We
> + * would prefer to use sizeof(FILE) here, but that is
> + * not guaranteed to be defined (e.g., FILE might be
> + * an incomplete type).
> + */
> + try_to_free_routine(1000);
> + stream = fdopen(fd, mode);
> + }
> +
> return stream;
> }
>
> +FILE *xfdopen(int fd, const char *mode)
> +{
> + FILE *stream = fdopen_with_retry(fd, mode);
> +
> + if (stream)
> + return stream;
> + else if (errno == ENOMEM)
> + die_errno("Out of memory? fdopen failed");
> + else
> + die_errno("fdopen failed");My gut feeling dislikes the distinction of different errors here just to pass in a different string to die. The die_errno will already print an appropriate message depending on the errno set. So I'd rather have no distinction here.
If you want to have a different message for the case of ENOMEM,
I'd rather put it into fdopen_with_retry, which then uses a loop or
counted goto [ I've seen that in the refs code:
if (nr_retries < MAX_RETRIES)
goto retry_again;
]Show 12 quoted lines
> +}
> +
> int xmkstemp(char *template)
> {
> int fd;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Michael Haggerty
