Re: [PATCH] config: support values longer than 1024 bytes
- From
Jeff King <peff@peff.net>
- Date
- Apr 6, 2011, 00:52 UTC
- Message-ID
- <20110406005223.GA10374@sigill.intra.peff.net>
- In-Reply-To
- <1302046203-4408-1-git-send-email-kusmabite@gmail.com>
On Wed, Apr 06, 2011 at 01:30:03AM +0200, Erik Faye-Lund wrote:
> The rather awkward return statement with strdup("") is because
> strbuf_detach returns NULL when there's nothing allocated. Even
> worse, it returns an uninitialized string if the string has been
> initialized with a non-zero 'hint'.That seems like two bugs in the strbuf code.
I would expect strbuf_detach to _always_ return an allocated buffer, even if it is xstrdup(""). Though the code in strbuf_detach is explicit about returning NULL rather than slopbuf, so perhaps there is a case where it is useful.
But for the other, one of the invariants of strbuf is that the string is always NUL-terminated. So I would expect strbuf_init to properly NUL-terminate after growing based on the hint.
Show 6 quoted lines
> static char *parse_value(void)
> {
> - static char value[1024];
> - int quote = 0, comment = 0, len = 0, space = 0;
> + struct strbuf value = STRBUF_INIT;
> + int quote = 0, comment = 0, space = 0;One thing you could about the strbuf_detach thing is to just use a single static strbuf instead of the static array, and just reset its length back to zero on each call. So:
static struct strbuf value = STRBUF_INIT; ... strbuf_reset(&value);
Show 5 quoted lines
> - value[len] = 0;
> - return value;
> + return value.len ?
> + strbuf_detach(&value, NULL) :
> + strdup("");Then this just becomes:
return value.buf;
> default: > + strbuf_release(&value); > return NULL;
And you can drop the release.
Show 8 quoted lines
> @@ -226,7 +226,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len) > if (!value) > return -1; > } > - return fn(name, value, data); > + ret = fn(name, value, data); > + free(value); > + return ret;
And drop this hunk, since callers no longer need to free.
I do wonder, though, if we could be reusing the unquote_c_style() function in quote.c. They are obviously similar, but I haven't checked if there is more going on in the config code.
-Peff