Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.

From: Kristian Høgsberg <krh@redhat.com>
Date: Sep 6, 2007, 18:18 UTC
Message-ID: <1189102714.3423.22.camel@hinata.boston.redhat.com>
In-Reply-To: <20070906180858.GJ8451@artemis.corp>

On Thu, 2007-09-06 at 20:08 +0200, Pierre Habouzit wrote:

Show 18 quoted lines

> On Thu, Sep 06, 2007 at 05:59:29PM +0000, Kristian Høgsberg wrote:
> > On Thu, 2007-09-06 at 13:20 +0200, Pierre Habouzit wrote:
> > > +	strbuf_grow(sb, len);
> > > +	strbuf_addf(sb, "%u %s=", len, keyword);
> > > +	strbuf_add(sb, value, valuelen);
> > > +	strbuf_addch(sb, '\n');
> > >  }
> > 
> > This entire function can be collapsed to just:
> > 
> > 	strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
> 
>   yes, but it's less efficient, because %.*s says that sprintf must copy
> at most valuelen bytes from value, but it still has to stop if it finds
> a \0 before. And the strbuf_grow has sense because the extend policy at
> snprintf time is optimistic: we try to write, and if it didn't fit, we
> try again. So there is a huge benefit if we have a clue of the final
> size.

Yeah, my strbuf_addf() suggestion doesn't work, since the format seems to indicate the embedded NULs indeed is allowed (the %u is the length of the value), so *.% could truncate data.

Show 13 quoted lines

> > > -	memcpy(path.buf, base, baselen);
> > > -	memcpy(path.buf + baselen, filename, filenamelen);
> > > -	path.len = baselen + filenamelen;
> > > -	path.buf[path.len] = '\0';
> > > +	strbuf_grow(&path, MAX(PATH_MAX, baselen + filenamelen + 1));
> > > +	strbuf_reset(&path);
> > 
> > Does strbuf_reset() do anything here?
> > 
> > > +	strbuf_add(&path, base, baselen);
> 
>   Yes _reset() sets length to 0. so the add here will write at the start
> of the buffer again. It definitely is important !

But where was length set to non-zero? path is initialized on entry to the function, and strbuf_grow() just increases the allocation, not length, right?

Kristian