Re: [PATCH v2 08/17] virstring: Introduce virStringIsNull()

On 8/20/20 1:02 PM, Peter Krempa wrote: > On Tue, Jul 07, 2020 at 21:46:26 +0200, Michal Privoznik wrote: > > This function will be used to detect zero buffers (which are > > going to be interpreted as hole in virStream later). > > > > I shamelessly took inspiration from coreutils. > > Coreutils is proudly GPLv3 ... > Sure. But it was discussed in v1 and I think we agreed that the algorithm is generic enough that it can be used in Libvirt too: https://www.redhat.com/archives/libvir-list/2020-July/msg00170.html > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > --- > > src/libvirt_private.syms | 1 + > > src/util/virstring.c | 38 ++++++++++++++++++++++++++++++++ > > src/util/virstring.h | 2 ++ > > tests/virstringtest.c | 47 ++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 88 insertions(+) > > [...] > > > diff --git a/src/util/virstring.c b/src/util/virstring.c > > index e9e792f3bf..c26bc770d4 100644 > > --- a/src/util/virstring.c > > +++ b/src/util/virstring.c > > @@ -1404,3 +1404,41 @@ int virStringParseYesNo(const char *str, bool *result) > > return 0; > > } > > + > > + > > +/** > > + * virStringIsNull: > > IsNull might indicate that this does a check if the pointer is NULL. You > are checking for NUL bytes. Fair enough. I'm out of ideas though. Do you have a suggestion? > > > + * @buf: buffer to check > > + * @len: the length of the buffer > > + * > > + * For given buffer @buf and its size @len determine whether > > + * it contains only zero bytes (NUL) or not. > > Given the semantics of C strings being terminated by the NUL byte I > don't think this function qualifies as a string helper and thus should > probably reside somewhere outside of virstring.h Alright. I will try to find better location. But since I want to use this function from virsh too I'd like to have it in utils. > > > + * > > + * Returns: true if buffer is full of zero bytes, > > + * false otherwise. > > + */ > > +bool virStringIsNull(const char *buf, size_t len) > > +{ > > + const char *p = buf; > > + > > + if (!len) > > + return true; > > + > > + /* Check up to 16 first bytes. */ > > + for (;;) { > > + if (*p) > > + return false; > > + > > + p++; > > + len--; > > + > > + if (!len) > > + return true; > > + > > + if ((len & 0xf) == 0) > > + break; > > + } > > Do we really need to do this optimization? We could arguably simplify > this to: > > if (*buf != '\0') > return false; > > return memcmp(buf, buf + 1, len - 1); > > You can then use the saved lines to explain that comparing a piece of > memory with itself shifted by any position just ensures that there are > repeating sequences of itself in the remainder and by shifting it by 1 > it means that it checks that the strings are just the same byte. The > check above then ensuers that the one byte is NUL. > The idea is to pass aligned address to memcmp(). But I guess we can let memcmp() deal with that.