Skip to content

[th/strbuf-v2]

Thomas Haller requested to merge th/strbuf-v2 into master

This is a rebase and resurrection of previous merge request !42 (closed) that got NACK'ed.

Sorry for bringing this up again. In general, I agree that once a thing is discussed and NACK'ed, that should be it.

However, I don't agree with the decision, and I'd like to re-discuss it again.

I don't wanna re-iterate the reasons here, let me just quote the commit message:

shared: add NMStrBuf util

Our own implementation of a string buffer like GString.

Advantages (in decreasing relevance):

- Since we are in control, we can easily let it nm_explicit_bzero()
  the memory. The regular GString API cannot be used in such a case.
  While nm_explicit_bzero() may or may not be of questionable benefit,
  the problem is that if the underlying API counteracts the aim of
  clearing memory, it gets impossible. As API like NMStrBuf supports
  it, clearing memory is a easy as enable the right flag.
  This would for example be useful for example when we read passwords
  from a file or file descriptor (e.g. try_spawn_vpn_auth_helper()).

- We have API like

    nmp_object_to_string (const NMPObject *obj,
                          NMPObjectToStringMode to_string_mode,
                          char *buf,
                          gsize buf_size);

  which accept a fixed size output buffer. This has the problem of
  how choosing the right sized buffer. With NMStrBuf such API could
  be instead

    nmp_object_to_string (const NMPObject *obj,
                          NMPObjectToStringMode to_string_mode,
                          NMStrBuf *buf);

  which can automatically grow (using heap allocation). It would be
  easy to extend NMStrBuf to use a fixed buffer or limiting the
  maximum string length. The point is, that the to-string API wouldn't
  have to change. Depending on the NMStrBuf passed in, you can fill
  an unbounded heap allocated string, a heap allocated string up to
  a fixed length, or a static string of fixed length. NMStrBuf currently
  only implements the unbounded heap allocate string case, but it would
  be simple to extend.

  Note that we already have API like nm_utils_strbuf_*() to fill a buffer
  of fixed size. GString is not useable for that (efficiently), hence
  this API exists. NMStrBuf could be easily extended to replace this API
  without usability or performance penalty. So, while this adds one new
  API, it could replace other APIs.

- GString always requires a heap allocation for the container. In by far
  most of the cases where we use GString, we use it to simply construct
  a string dynamically. There is zero use for this overhead. If one
  really needs a heap allocated buffer, NMStrBuf can easily embedded
  in a malloc'ed memory and boxed that way.

- GString API supports inserting and removing range. We almost never
  make use of that. We only require append-only, which is simple to
  implement.

- GString needs to NUL terminate the buffer on every append. It
  has unnecessary overhead for allowing a usage of where intermediate
  buffer contents are valid strings too. That is not the case with
  NMStrBuf: the API requires the user to call nm_str_buf_get_str() or
  nm_str_buf_finalize(). In most cases, you would only access the string
  once at the end, and not while constructing it.

- GString always grows the buffer size by doubling it. I don't think
  that is optimal. I don't think there is one optimal approach for how
  to grow the buffer, it depends on the usage patterns. However, trying
  to make an optimal choice here makes a difference. QT also thinks so,
  and I adopted their approach in nm_utils_get_next_realloc_size().

Merge request reports