[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]