From: Tomi Ollila <tomi.ollila@iki.fi>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [Patch v3] util: add gzreadline
In-Reply-To: <1396178602-7583-1-git-send-email-david@tethera.net>
References: <m2zjk8um02.fsf@guru.guru-group.fi>
	<1396178602-7583-1-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.17+171~g9a127b7 (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Sun, 30 Mar 2014 15:45:21 +0300
Message-ID: <m2txafvori.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Sun, Mar 30 2014, David Bremner <david@tethera.net> wrote:

> The idea is to provide a more or less drop in replacement for readline
> to read from zlib/gzip streams.  Take the opportunity to replace
> malloc with talloc.
> ---
>
> This corrects one nasty bug, and a few style/optimization issues brought up by Tomi.
>
> I'm not sure about the name. I agree in principle it would be good to
> signal "talloc". I'm just not sure the best way to do that.

how about just gz_getline(). If there were gzgetline() one would expect it's
function "signature" be gzgetline(char **lineptr, size_t *m gzFile *stream);
(consistent with other '' -> gz'' conversions). for gz_getline() there
would be no expectations.

>
> I'm also not sure about the error handling. the "real" getline sets
> errno. Should we?

In comparison with gzgets() return value

"On success, gzgets() shall return a pointer to buf. Otherwise, gzgets()
"shall return Z_NULL. Applications may examine the cause using gzerror().
"
"Errors
"
"On error, gzgets() shall return Z_NULL.

In light of this I'd say no. can we set out-of-memory using some public
api so it is available using gzerror()


Rest of the style issues inline ;p

Tomi

>
>  util/Makefile.local |  2 +-
>  util/zlib-extra.c   | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/zlib-extra.h   | 10 +++++++++
>  3 files changed, 73 insertions(+), 1 deletion(-)
>  create mode 100644 util/zlib-extra.c
>  create mode 100644 util/zlib-extra.h
>
> diff --git a/util/Makefile.local b/util/Makefile.local
> index 29c0ce6..e2a5b65 100644
> --- a/util/Makefile.local
> +++ b/util/Makefile.local
> @@ -4,7 +4,7 @@ dir := util
>  extra_cflags += -I$(srcdir)/$(dir)
>  
>  libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
> -		  $(dir)/string-util.c $(dir)/talloc-extra.c
> +		  $(dir)/string-util.c $(dir)/talloc-extra.c $(dir)/zlib-extra.c
>  
>  libutil_modules := $(libutil_c_srcs:.c=.o)
>  
> diff --git a/util/zlib-extra.c b/util/zlib-extra.c
> new file mode 100644
> index 0000000..7afe175
> --- /dev/null
> +++ b/util/zlib-extra.c
> @@ -0,0 +1,62 @@
> +/* zlib-extra.c -  Extra or enhanced routines for compressed I/O.
> + *
> + * Copyright (c) 2014 David Bremner
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: David Bremner <david@tethera.net>
> + */
> +
> +#include "zlib-extra.h"
> +#include <talloc.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +/* mimic POSIX/glibc getline, but on a zlib gzFile stream, and using talloc */
> +ssize_t
> +gzgetline (void *ctx, char **lineptr, size_t *n, gzFile stream) {
> +

opening brace ({) starting function content to it's own line.

> +    size_t len = *n;
> +    char *buf = *lineptr;
> +    size_t offset = 0;
> +
> +    if (len == 0 || buf == NULL) {
> +	/* same as getdelim from gnulib */
> +	len = 120;
> +	buf = talloc_size (ctx, len);
> +	if (buf == NULL)
> +	    return -1;
> +    }
> +
> +    while (1) {
> +

extra empty line ?

> +	if (!gzgets (stream, buf + offset, len - offset))

! gzgets

> +	    return -1;
> +
> +	offset += strlen (buf+offset);

(buf + offset)

> +
> +	if ( buf[offset-1] == '\n' )

[ offset - 1 ]

> +	    break;
> +
> +	len *= 2;
> +	buf = talloc_realloc (ctx, buf, char, len);
> +	if (buf == NULL)
> +	    return -1;
> +

extra empty line

> +    }
> +
> +    *lineptr = buf;
> +    *n = len;
> +    return offset;
> +}
> diff --git a/util/zlib-extra.h b/util/zlib-extra.h
> new file mode 100644
> index 0000000..c18480f
> --- /dev/null
> +++ b/util/zlib-extra.h
> @@ -0,0 +1,10 @@
> +#ifndef _ZLIB_EXTRA_H
> +#define _ZLIB_EXTRA_H
> +
> +#include <zlib.h>
> +
> +/* Like getline, but read from a gzFile. Allocation is with talloc */
> +ssize_t
> +gzgetline (void *ctx, char **lineptr, size_t *n, gzFile stream);
> +
> +#endif
> -- 
> 1.9.0