Re: [dm-devel] [PATCH 2/9] libmultipath: strbuf: simple api for growing string buffers

[Martin Wilck <mwilck@suse.com>]

On Mo, 2021-07-26 at 23:54 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 15, 2021 at 12:52:16PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Add an API for string buffers that grow in size as text is added.
> > This API will be useful in several places of the multipath-tools
> > code
> > base. Add unit tests for these helpers, too.
> > 
> 
> This looks good. I have a couple of nitpicks, but none of them are
> actually bugs, so if you prefer what you have, you can ignore them.
> 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/Makefile |   2 +-
> >  libmultipath/strbuf.c | 207 +++++++++++++++++++++
> >  libmultipath/strbuf.h | 168 +++++++++++++++++
> >  tests/Makefile        |   3 +-
> >  tests/strbuf.c        | 412
> > ++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 790 insertions(+), 2 deletions(-)
> >  create mode 100644 libmultipath/strbuf.c
> >  create mode 100644 libmultipath/strbuf.h
> >  create mode 100644 tests/strbuf.c
> > 
> > diff --git a/libmultipath/Makefile b/libmultipath/Makefile
> > index e7254f3..7f3921c 100644
> > --- a/libmultipath/Makefile
> > +++ b/libmultipath/Makefile
> > @@ -53,7 +53,7 @@ OBJS = memory.o parser.o vector.o devmapper.o
> > callout.o \
> >         log.o configure.o structs_vec.o sysfs.o prio.o checkers.o \
> >         lock.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o \
> >         io_err_stat.o dm-generic.o generic.o foreign.o nvme-lib.o \
> > -       libsg.o valid.o
> > +       libsg.o valid.o strbuf.o
> >  
> >  all:   $(DEVLIB)
> >  
> > diff --git a/libmultipath/strbuf.c b/libmultipath/strbuf.c
> > new file mode 100644
> > index 0000000..8422a50
> > --- /dev/null
> > +++ b/libmultipath/strbuf.c

> > +
> > +static int expand_strbuf(struct strbuf *buf, int addsz)
> > +{
> > +       size_t add;
> > +       char *tmp;
> > +
> > +       assert(strbuf_is_sane(buf));
> > +       if (addsz < 0)
> > +               return -EINVAL;
> > +       if (buf->size - buf->offs >= (size_t)addsz + 1)
> > +               return 0;
> > +
> 
> With this add calculation, if you need exactly (x) BUF_CHUNKS worth
> of
> space, you will add (x + 1) BUF_CHUNKS worth of space.  If you
> instead
> use
> 
> add = ((addsz - (buf->size - buf->offs)) / BUF_CHUNK + 1)
> 
> you will avoid allocating the extra BUF_CHUNK.

You are right, thanks. I was erring on the safe side, but still erring.

> 
> > +       add = ((addsz + 1 - (buf->size - buf->offs)) / BUF_CHUNK +
> > 1)
> > +               * BUF_CHUNK;
> > +
> > +       if (buf->size >= SIZE_MAX - add) {
> > +               add = SIZE_MAX - buf->size;
> > +               if (add < (size_t)addsz + 1)
> > +                       return -EOVERFLOW;
> > +       }
> > +
> > +       tmp = realloc(buf->buf, buf->size + add);
> > +       if (!tmp)
> > +               return -ENOMEM;
> > +
> > +       buf->buf = tmp;
> > +       buf->size += add;
> > +       buf->buf[buf->offs] = '\0';
> > +
> > +       return 0;
> > +}
> > +
> > +int __append_strbuf_str(struct strbuf *buf, const char *str, int
> > slen)
> > +{
> > +       int ret;
> > +
> > +       if ((ret = expand_strbuf(buf, slen)) < 0)
> > +               return ret;
> > +
> 
> This is correct, but I think it would be clearer if you calculated
> buf->offs like you do in fill_strbufi():
> 
>         memcpy(buf->buf + buf->offs, str, slen);
>         buf->offs += slen;

Ok.

> > +       buf->offs = (char *)mempcpy(buf->buf + buf->offs, str,
> > slen)
> > +               - buf->buf;
> > +       buf->buf[buf->offs] = '\0';
> > +
> > +       return slen;
> > +}
> > +

> > +
> > +int append_strbuf_quoted(struct strbuf *buff, const char *ptr)
> > +{
> > +       char *quoted, *q;
> > +       const char *p;
> > +       unsigned n_quotes, i;
> > +       size_t qlen;
> > +       int ret;
> > +
> > +       if (!ptr)
> > +               return -EINVAL;
> > +
> > +       for (n_quotes = 0, p = strchr(ptr, '"'); p; p = strchr(++p,
> > '"'))
> > +               n_quotes++;
> > +
> > +       /* leading + trailing quote, 1 extra quote for every quote
> > in ptr */
> > +       qlen = strlen(ptr) + 2 + n_quotes;
> > +       if (qlen > INT_MAX)
> > +               return -ERANGE;
> > +       if ((ret = expand_strbuf(buff, qlen)) < 0)
> > +               return ret;
> > +
> > +       quoted = &(buff->buf[buff->offs]);
> > +       *quoted++ = '"';
> > +       for (p = ptr, q = quoted, i = 0; i < n_quotes; i++) {
> 
> "qlen - 2 - (q - quoted)" is likely more space then you need here,
> although it doesn't matter, since you already know that you will find
> a
> quote before the end of the string.

qlen - 2 - (q - quoted) is the available space in the destination
buffer, (2 because I must append another quote after processing).

>  I get that it matches the
> calculation after the loop. but even that one is confusing. I would
> prefer that you either did something like
> 
>         size_t len = strlen(ptr);
> 
> ...
> 
>                 char *q1 = memccpy(q, p, '"', len - (p - ptr));
> 
> or even more obviously correct
> 
>         const char *p_end = ptr + strlen(ptr);
> 
> ...
> 
>                 char *q1 = memccpy(q, p, '"', p_end - p);

Is it really more obvious? Your suggestion uses the pointers from the
source string, my code uses those from the destination string. The two
approaches are equivalent at a closer look. IMO it's more important to
make it obvious that we don't write beyond the end of the destination
than to show that we don't read beyond the end of the source (assuming
that write overflows do more harm than read overflows).

So in this specific case, I'd like to keep my version.

Thanks,
Martin



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel