Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 3385 bytes --]

On Fri, Oct 05, 2007 at 03:26:44PM +0000, Linus Torvalds wrote:
> 
> 
> On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> >  
> > -	strbuf_grow(buf, len);
> > +	/* only grow if not in place */
> > +	if (strbuf_avail(buf) + buf->len < len)
> > +		strbuf_grow(buf, len - buf->len);
> 
> Umm. This is really ugly.

  I agree.

> The whole point of strbuf's was that you shouldn't be doing your own 
> allocation decisions etc. So why do it?

  the point here is that it's in a "filter" that is called like this:

some_filter(buf->buf, buf->len, buf);
            src       len       dst

  You can call the filter with src/len being data from anywere,
including the current content of the destination buffer.

  Then there is two cases, either the filter is known to be done in
place, either we can't know or we know it wont.

  In the latter case, we have a bit of code like that:

      char *to_free = NULL;

      if (buf->buf == src)
	  to_free = strbuf_detach(&buf);

      .. hack ..
      free(to_free);


  In the former case, then there is a small glitch, being that if we are
doing in place editing, we should not touch buffer at all (or it would
invalidate "src"). If we are not in the in-place editing code though,
then we have to make the resulting buffer be big enough...

> Wouldn't it be much better to have a strbuf_make_room() interface that 
> just guarantees that there is enough room fo "len"? 

  Right, that would do the same btw ;)

> Otherwise, code like the above would seem to make the whole point of a 
> safer string interface rather pointless. The above code only makes sense 
> if you know how the strbuf's are internally done, so it should not exists 
> except as internal strbuf code. No?

  Well, the above code is used in filters to spare reallocations. So if
we want to "blackbox" such a think, strbuf_make_room isn't the proper
API. We should rather use
  void *strbuf_begin_filter(struct strbuf *sb, const char *src, size_t reslen);
  strbuf_end_filter(void *);

  `strbuf_begin_filter` would decide upon the hint `reslen` argument if
we know if we can work in place or not (has a meaning iff src points
into the strbuf buffer). If not, it could stash the strbuf buffer in the
returned void * to be freed at the end of the filter. It seems like a
better alternative than a strbuf_make_room.

  Of course, strbuf_begin_filter() would really be simple and basically
be:

    char *tmp;
    if (src points into sb->buf && reslen > sb->alloc - 1) {
        // in place editing is OK
        return NULL;
    }
    tmp = strbuf_release(&sb);
    strbuf_grow(&sb, len);
    return tmp;


  and strbuf_end_filter would just be "free" :)


  We could even make "reslen" be a ssize_t so that -1 would mean "I've
absolutely no idea how much space I'll need (or just in place editing is
not supported). This way, both hacks I described in this mail could be
hidden in the strbuf module, and be properly documented _and_ safe _and_
efficient.

What do you think ?

[Though if we do that, I still think it's more important to fix the
bug in master, and have a new patch implementing this approach]
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]