Re: [Qemu-devel] [PATCH v7 1/2] util: add memmem replacement function

[Markus Armbruster]

address@hidden writes:

> From: Claudio Fontana <address@hidden>
>
> if the memmem function is missing, provide a trivial replacement.
>
> Signed-off-by: Claudio Fontana <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> Acked-by: Luiz Capitulino <address@hidden>
> ---
>  configure            | 15 +++++++++++++
>  include/qemu/osdep.h |  4 ++++
>  util/Makefile.objs   |  1 +
>  util/memmem.c        | 63 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
>  create mode 100644 util/memmem.c
>
> diff --git a/configure b/configure
> index 409edf9..9e04269 100755
> --- a/configure
> +++ b/configure
> @@ -3091,6 +3091,17 @@ if compile_prog "" "" ; then
>  fi
>  
>  ##########################################
> +# memmem probe
> +cat > $TMPC <<EOF
> +#include <string.h>
> +int main(int argc, char *argv[]) { return memmem(argv[0], 0, argv[0], 0) != 
> argv[0]; }
> +EOF
> +memmem=no
> +if compile_prog "" "" ; then
> +  memmem=yes
> +fi
> +
> +##########################################
>  # fdt probe
>  # fdt support is mandatory for at least some target architectures,
>  # so insist on it if we're building those system emulators.
> @@ -4473,6 +4484,7 @@ echo "RDMA support      $rdma"
>  echo "TCG interpreter   $tcg_interpreter"
>  echo "fdt support       $fdt"
>  echo "preadv support    $preadv"
> +echo "memmem support    $memmem"
>  echo "fdatasync         $fdatasync"
>  echo "madvise           $madvise"
>  echo "posix_madvise     $posix_madvise"
> @@ -4822,6 +4834,9 @@ fi
>  if test "$preadv" = "yes" ; then
>    echo "CONFIG_PREADV=y" >> $config_host_mak
>  fi
> +if test "$memmem" = "yes" ; then
> +  echo "CONFIG_MEMMEM=y" >> $config_host_mak
> +fi
>  if test "$fdt" = "yes" ; then
>    echo "CONFIG_FDT=y" >> $config_host_mak
>  fi
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 3247364..abc5486 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -201,6 +201,10 @@ ssize_t writev(int fd, const struct iovec *iov, int 
> iov_cnt);
>  #include <sys/uio.h>
>  #endif
>  
> +#ifndef CONFIG_MEMMEM
> +void *memmem(const void *hay, size_t hay_len, const void *s, size_t s_len);
> +#endif /* !CONFIG_MEMMEM */
> +
>  #ifdef _WIN32
>  static inline void qemu_timersub(const struct timeval *val1,
>                                   const struct timeval *val2,
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index ceaba30..628242f 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -1,6 +1,7 @@
>  util-obj-y = osdep.o cutils.o unicode.o qemu-timer-common.o
>  util-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o 
> event_notifier-win32.o
>  util-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o 
> event_notifier-posix.o qemu-openpty.o
> +util-obj-$(call lnot,$(CONFIG_MEMMEM)) += memmem.o
>  util-obj-y += envlist.o path.o module.o
>  util-obj-$(call lnot,$(CONFIG_INT128)) += host-utils.o
>  util-obj-y += bitmap.o bitops.o hbitmap.o
> diff --git a/util/memmem.c b/util/memmem.c
> new file mode 100644
> index 0000000..16ac611
> --- /dev/null
> +++ b/util/memmem.c
> @@ -0,0 +1,63 @@
> +/*
> + * memmem replacement function
> + *
> + * Copyright (C) 2015 Huawei Technologies Duesseldorf GmbH
> + * Written by Claudio Fontana <address@hidden>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include <qemu-common.h>

#include "qemu-common.h", please, like everywhere else.

> +
> +/*
> + * Search for the first occurrence of a binary string ("needle")
> + * in a memory region ("haystack").
> + *
> + * If needle length is 0, it returns the pointer to the haystack.
> + * Otherwise it returns the pointer to the first character of the first
> + * occurrence of the needle in the haystack, or NULL if none are found.
> + *
> + */
> +void *
> +memmem(const void *haystack, size_t hay_len, const void *needle, size_t 
> s_len)

I prefer function comments to follow a strict pattern:

    /*
     * Headline explaining the function's purpose[*]
     * Zero or more paragraphs explaining preconditions, side effects,
     * return values, error conditions.
     */

[*] If you can't come up with a headline fitting into a single line,
chances are the function does too many things.

Calling the size of @needle @s_len is odd.

What about:

/*
 * Return the first occurrence of @needle in @haystack, or else NULL.
 * @needle consists of @needle_sz bytes.
 * @haystack consists of @haystack_sz bytes.
 * Note: returns @haystack when @needle_sz is zero, because an empty
 * needle matches anywhere.
 */
void *
memmem(const void *haystack, size_t haystack_sz,
       const void *needle, size_t needle_sz)

> +{
> +    const unsigned char *hay = (const unsigned char *)haystack;
> +    const unsigned char *s = (const unsigned char *)needle;

Superfluous casts.

> +    const unsigned char *last = hay + (hay_len - s_len);
> +
> +    if (s_len == 0) {
> +        return (void *)hay;
> +    }
> +
> +    if (hay_len < s_len) {
> +        return NULL;
> +    }
> +
> +    if (s_len == 1) {
> +        return memchr(hay, s[0], hay_len);
> +    }

Why special-case length 1?

> +
> +    for (; hay <= last; hay++) {
> +        if (hay[0] == s[0] && memcmp(hay, s, s_len) == 0) {

Why not just memcmp()?

> +            return (void *)hay;
> +        }
> +    }
> +
> +    return NULL;
> +}