On Thu, Aug 20, 2020 at 14:36:15 +0200, Michal Privoznik wrote:
> 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 at redhat.com>
> > > ---
> > > 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.
Well, this explanation might justify the algorithm above, but it's
certainly not obvious, so please add a comment.