Re: [Patch v2 2/3] util: add gzreadline

[Tomi Ollila <tomi.ollila@iki.fi>]

On Sat, Mar 29 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 series looks pretty good to me. I thought I found a place for real
improvement but I was wrong. Therefore I look in this for more detail ;D

>  util/Makefile.local |  2 +-
>  util/zlib-extra.c   | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/zlib-extra.h   | 10 +++++++++
>  3 files changed, 70 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..cda369f
> --- /dev/null
> +++ b/util/zlib-extra.c
> @@ -0,0 +1,59 @@
> +/* 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) {

The naming space of this function looks like the gz*functions in general;
But this adds a special talloc usage. getline() > gzgetline() conversion
should be consistent w/ other gz*() functions. I'd suggest to make slight
change to the function name to sanction the talloc usage. Ah, the fine
art & pain in naming considerations...

> +
> +    size_t len=*n;

spaces around '='

> +    char *buf = *lineptr;
> +    size_t offset = 0;
> +
> +    if (len == 0 || buf == NULL) {
> +	/* XXX arbitrary choice of initial size */

what does getline() use there.. could the 'XXX' just be dropped.

> +	len = BUFSIZ;
> +	buf = talloc_size (ctx, len);
> +    }
> +
> +    while (1) {
> +
> +	if (!gzgets (stream, buf + offset, len))
> +	    return -1;
> +
> +	/* XXX wasted effort re-counting length of whole line */
> +	offset = strlen (buf);

offset += strlen (buf + offset); would drop the whole line recounting...
just would that ever happen (gzgets() just usually returns the whole line
with '\n').. so many alternatives there...

...maybe just comment the usual case that full lines are returned.


Tomi


> +
> +	if ( buf[offset-1] == '\n' )
> +	    break;
> +
> +	len *= 2;
> +	buf = talloc_realloc (ctx, buf, char, len);
> +
> +    }
> +
> +    *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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch