[PATCH 2/9] virstring: Introduce virStringIsNull()

Michal Privoznik posted 9 patches 5 years, 7 months ago
There is a newer version of this series
[PATCH 2/9] virstring: Introduce virStringIsNull()
Posted by Michal Privoznik 5 years, 7 months ago
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.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virstring.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 src/util/virstring.h     |  2 ++
 3 files changed, 43 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ae0e253ab9..1d80aeb833 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3197,6 +3197,7 @@ virStringHasChars;
 virStringHasControlChars;
 virStringHasSuffix;
 virStringIsEmpty;
+virStringIsNull;
 virStringIsPrintable;
 virStringListAdd;
 virStringListAutoFree;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index e9e792f3bf..7f6871d5ab 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1404,3 +1404,43 @@ int virStringParseYesNo(const char *str, bool *result)
 
     return 0;
 }
+
+
+/**
+ * virStringIsNull:
+ * @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 or not.
+ *
+ * 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. memcmp() below uses block of sizeof(long)
+     * size (in most cases on 64bits is 8 bytes), doing twice the size just to
+     * be safe. */
+    for (;;) {
+        if (*p)
+            return false;
+
+        p++;
+        len--;
+
+        if (!len)
+            return true;
+
+        if ((len & (2 * sizeof(long))) == 0)
+            break;
+    }
+
+    /* Now we know first 16 bytes are NUL, memcmp with self.  */
+    return memcmp(buf, p, len) == 0;
+}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 360c68395c..d0e54358ac 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -185,6 +185,8 @@ int virStringParsePort(const char *str,
 int virStringParseYesNo(const char *str,
                         bool *result)
     G_GNUC_WARN_UNUSED_RESULT;
+bool virStringIsNull(const char *buf, size_t len);
+
 /**
  * VIR_AUTOSTRINGLIST:
  *
-- 
2.26.2

Re: [PATCH 2/9] virstring: Introduce virStringIsNull()
Posted by Ján Tomko 5 years, 7 months ago
On a Friday in 2020, 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.
>

How is that okay, isn't coreutils GPL-licensed?

Jano

>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/libvirt_private.syms |  1 +
> src/util/virstring.c     | 40 ++++++++++++++++++++++++++++++++++++++++
> src/util/virstring.h     |  2 ++
> 3 files changed, 43 insertions(+)
>
Re: [PATCH 2/9] virstring: Introduce virStringIsNull()
Posted by Michal Privoznik 5 years, 7 months ago
On 7/3/20 2:08 PM, Ján Tomko wrote:
> On a Friday in 2020, 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.
>>
> 
> How is that okay, isn't coreutils GPL-licensed?

Well, I only took inspiration in that code, I haven't copied it.
But this idea of using memcmp() is quite old and described for instance 
in this blog post too:

https://rusty.ozlabs.org/?p=560

Do you want to switch to something else instead? If so, how can we make 
sure it's not already somewhere in a code with incompatible license?

Michal

Re: [PATCH 2/9] virstring: Introduce virStringIsNull()
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Fri, Jul 03, 2020 at 03:13:29PM +0200, Michal Privoznik wrote:
> On 7/3/20 2:08 PM, Ján Tomko wrote:
> > On a Friday in 2020, 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.
> > > 
> > 
> > How is that okay, isn't coreutils GPL-licensed?
> 
> Well, I only took inspiration in that code, I haven't copied it.
> But this idea of using memcmp() is quite old and described for instance in
> this blog post too:
> 
> https://rusty.ozlabs.org/?p=560
> 
> Do you want to switch to something else instead? If so, how can we make sure
> it's not already somewhere in a code with incompatible license?

I think the current code is fine as it is simple logic. If someone
described the algorithm you'd end up doing it the same way

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/9] virstring: Introduce virStringIsNull()
Posted by Daniel P. Berrangé 5 years, 7 months ago
On Fri, Jul 03, 2020 at 12:28:43PM +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.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virstring.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>  src/util/virstring.h     |  2 ++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ae0e253ab9..1d80aeb833 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3197,6 +3197,7 @@ virStringHasChars;
>  virStringHasControlChars;
>  virStringHasSuffix;
>  virStringIsEmpty;
> +virStringIsNull;
>  virStringIsPrintable;
>  virStringListAdd;
>  virStringListAutoFree;
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index e9e792f3bf..7f6871d5ab 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -1404,3 +1404,43 @@ int virStringParseYesNo(const char *str, bool *result)
>  
>      return 0;
>  }
> +
> +
> +/**
> + * virStringIsNull:
> + * @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 or not.
> + *
> + * 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. memcmp() below uses block of sizeof(long)
> +     * size (in most cases on 64bits is 8 bytes), doing twice the size just to
> +     * be safe. */

On 32-bit this is only going to do 8 bytes though. I'm not sure
why we need "2 * sizeof(long)" instead of "16". Unless we really
do want this to have different behaviour based on sizeof(long),
in which case the comment could be clearer

> +    for (;;) {
> +        if (*p)
> +            return false;
> +
> +        p++;
> +        len--;
> +
> +        if (!len)
> +            return true;
> +
> +        if ((len & (2 * sizeof(long))) == 0)
> +            break;
> +    }
> +
> +    /* Now we know first 16 bytes are NUL, memcmp with self.  */
> +    return memcmp(buf, p, len) == 0;
> +}
> diff --git a/src/util/virstring.h b/src/util/virstring.h
> index 360c68395c..d0e54358ac 100644
> --- a/src/util/virstring.h
> +++ b/src/util/virstring.h
> @@ -185,6 +185,8 @@ int virStringParsePort(const char *str,
>  int virStringParseYesNo(const char *str,
>                          bool *result)
>      G_GNUC_WARN_UNUSED_RESULT;
> +bool virStringIsNull(const char *buf, size_t len);
> +
>  /**
>   * VIR_AUTOSTRINGLIST:
>   *
> -- 
> 2.26.2
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|