[Qemu-devel] [PATCH v4] cutils: Provide strchrnul

Keno Fischer posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1528653731-4920-1-git-send-email-keno@juliacomputing.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
There is a newer version of this series
configure             | 15 +++++++++++++++
hmp.c                 |  8 ++++----
hw/9pfs/9p-local.c    |  2 +-
include/qemu/cutils.h |  8 ++++++++
monitor.c             |  8 ++------
util/cutils.c         | 15 +++++++++++++++
util/qemu-option.c    |  6 +-----
util/uri.c            |  6 ++----
8 files changed, 48 insertions(+), 20 deletions(-)
[Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Keno Fischer 5 years, 9 months ago
strchrnul is a GNU extension and thus unavailable on a number of targets.
In the review for a commit removing strchrnul from 9p, I was asked to
create a qemu_strchrnul helper to factor out this functionality.
Do so, and use it in a number of other places in the code base that inlined
the replacement pattern in a place where strchrnul could be used.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
Acked-by: Greg Kurz <groug@kaod.org>
---

Changes since v3:
 - Fix bug in configure check

 configure             | 15 +++++++++++++++
 hmp.c                 |  8 ++++----
 hw/9pfs/9p-local.c    |  2 +-
 include/qemu/cutils.h |  8 ++++++++
 monitor.c             |  8 ++------
 util/cutils.c         | 15 +++++++++++++++
 util/qemu-option.c    |  6 +-----
 util/uri.c            |  6 ++----
 8 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/configure b/configure
index 14b1113..f5ca850 100755
--- a/configure
+++ b/configure
@@ -4747,6 +4747,18 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# check if we have strchrnul
+
+strchrnul=no
+cat > $TMPC << EOF
+#include <string.h>
+int main(void) { (void)strchrnul("Hello World", 'W'); return 0; }
+EOF
+if compile_prog "" "" ; then
+    strchrnul=yes
+fi
+
+##########################################
 # check if trace backend exists
 
 $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" --check-backends  > /dev/null 2> /dev/null
@@ -6229,6 +6241,9 @@ fi
 if test "$sem_timedwait" = "yes" ; then
   echo "CONFIG_SEM_TIMEDWAIT=y" >> $config_host_mak
 fi
+if test "$strchrnul" = "yes" ; then
+  echo "CONFIG_STRCHRNUL=y" >> $config_host_mak
+fi
 if test "$byteswap_h" = "yes" ; then
   echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
 fi
diff --git a/hmp.c b/hmp.c
index ef93f48..416d4c9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2123,12 +2123,12 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
     int has_hold_time = qdict_haskey(qdict, "hold-time");
     int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
     Error *err = NULL;
-    char *separator;
+    const char *separator;
     int keyname_len;
 
     while (1) {
-        separator = strchr(keys, '-');
-        keyname_len = separator ? separator - keys : strlen(keys);
+        separator = qemu_strchrnul(keys, '-');
+        keyname_len = separator - keys;
 
         /* Be compatible with old interface, convert user inputted "<" */
         if (keys[0] == '<' && keyname_len == 1) {
@@ -2165,7 +2165,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
             keylist->value->u.qcode.data = idx;
         }
 
-        if (!separator) {
+        if (!*separator) {
             break;
         }
         keys = separator + 1;
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 5721eff..828e8d6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
         assert(*path != '/');
 
         head = g_strdup(path);
-        c = strchrnul(path, '/');
+        c = qemu_strchrnul(path, '/');
         if (*c) {
             /* Intermediate path element */
             head[c - path] = 0;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index a663340..809090c 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -122,6 +122,14 @@ int qemu_strnlen(const char *s, int max_len);
  * Returns: the pointer originally in @input.
  */
 char *qemu_strsep(char **input, const char *delim);
+#ifdef CONFIG_STRCHRNUL
+static inline const char *qemu_strchrnul(const char *s, int c)
+{
+    return strchrnul(s, c);
+}
+#else
+const char *qemu_strchrnul(const char *s, int c);
+#endif
 time_t mktimegm(struct tm *tm);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
diff --git a/monitor.c b/monitor.c
index 6d0cec5..4484d74 100644
--- a/monitor.c
+++ b/monitor.c
@@ -797,9 +797,7 @@ static int compare_cmd(const char *name, const char *list)
     p = list;
     for(;;) {
         pstart = p;
-        p = strchr(p, '|');
-        if (!p)
-            p = pstart + strlen(pstart);
+        p = qemu_strchrnul(p, '|');
         if ((p - pstart) == len && !memcmp(pstart, name, len))
             return 1;
         if (*p == '\0')
@@ -3400,9 +3398,7 @@ static void cmd_completion(Monitor *mon, const char *name, const char *list)
     p = list;
     for(;;) {
         pstart = p;
-        p = strchr(p, '|');
-        if (!p)
-            p = pstart + strlen(pstart);
+        p = qemu_strchrnul(p, '|');
         len = p - pstart;
         if (len > sizeof(cmd) - 2)
             len = sizeof(cmd) - 2;
diff --git a/util/cutils.c b/util/cutils.c
index 0de69e6..c365ddb 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -545,6 +545,21 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
 }
 
 /**
+ * Searches for the first occurrence of 'c' in 's', and returns a pointer
+ * to the trailing null byte if none was found.
+ */
+#ifndef CONFIG_STRCHRNUL
+const char *qemu_strchrnul(const char *s, int c)
+{
+    const char *e = strchr(s, c);
+    if (!e) {
+        e = s + strlen(s);
+    }
+    return e;
+}
+#endif
+
+/**
  * parse_uint:
  *
  * @s: String to parse
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 58d1c23..54eca12 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -77,11 +77,7 @@ const char *get_opt_value(const char *p, char **value)
 
     *value = NULL;
     while (1) {
-        offset = strchr(p, ',');
-        if (!offset) {
-            offset = p + strlen(p);
-        }
-
+        offset = qemu_strchrnul(p, ',');
         length = offset - p;
         if (*offset != '\0' && *(offset + 1) == ',') {
             length++;
diff --git a/util/uri.c b/util/uri.c
index 8624a7a..8bdef84 100644
--- a/util/uri.c
+++ b/util/uri.c
@@ -52,6 +52,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 
 #include "qemu/uri.h"
 
@@ -2266,10 +2267,7 @@ struct QueryParams *query_params_parse(const char *query)
         /* Find the next separator, or end of the string. */
         end = strchr(query, '&');
         if (!end) {
-            end = strchr(query, ';');
-        }
-        if (!end) {
-            end = query + strlen(query);
+            end = qemu_strchrnul(query, ';');
         }
 
         /* Find the first '=' character between here and end. */
-- 
2.8.1


Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Markus Armbruster 5 years, 9 months ago
Keno Fischer <keno@juliacomputing.com> writes:

> strchrnul is a GNU extension and thus unavailable on a number of targets.
> In the review for a commit removing strchrnul from 9p, I was asked to
> create a qemu_strchrnul helper to factor out this functionality.
> Do so, and use it in a number of other places in the code base that inlined
> the replacement pattern in a place where strchrnul could be used.
>
> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> Acked-by: Greg Kurz <groug@kaod.org>
> ---
>
> Changes since v3:
>  - Fix bug in configure check
>
>  configure             | 15 +++++++++++++++
>  hmp.c                 |  8 ++++----
>  hw/9pfs/9p-local.c    |  2 +-
>  include/qemu/cutils.h |  8 ++++++++
>  monitor.c             |  8 ++------
>  util/cutils.c         | 15 +++++++++++++++
>  util/qemu-option.c    |  6 +-----
>  util/uri.c            |  6 ++----
>  8 files changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/configure b/configure
> index 14b1113..f5ca850 100755
> --- a/configure
> +++ b/configure
> @@ -4747,6 +4747,18 @@ if compile_prog "" "" ; then
>  fi
>  
>  ##########################################
> +# check if we have strchrnul
> +
> +strchrnul=no
> +cat > $TMPC << EOF
> +#include <string.h>
> +int main(void) { (void)strchrnul("Hello World", 'W'); return 0; }

Suggest return strchrnul("Hello World", 'W') != 6, to avoid worries
about a sufficiently smart compilers optimizing out a call that would
otherwise fail to link, say because headers don't match libraries.

> +EOF
> +if compile_prog "" "" ; then
> +    strchrnul=yes
> +fi
> +
> +##########################################
>  # check if trace backend exists
>  
>  $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" --check-backends  > /dev/null 2> /dev/null

You're not printing $strchrnul like we print other configuration
results.  Hmm, we're not printing several of them.  Question for
maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three
coughed up by get_maintainer.pl): bug or feature?  If feature, how do we
decide what to print?

> @@ -6229,6 +6241,9 @@ fi
>  if test "$sem_timedwait" = "yes" ; then
>    echo "CONFIG_SEM_TIMEDWAIT=y" >> $config_host_mak
>  fi
> +if test "$strchrnul" = "yes" ; then
> +  echo "CONFIG_STRCHRNUL=y" >> $config_host_mak
> +fi
>  if test "$byteswap_h" = "yes" ; then
>    echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
>  fi
> diff --git a/hmp.c b/hmp.c
> index ef93f48..416d4c9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2123,12 +2123,12 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>      int has_hold_time = qdict_haskey(qdict, "hold-time");
>      int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
>      Error *err = NULL;
> -    char *separator;
> +    const char *separator;
>      int keyname_len;
>  
>      while (1) {
> -        separator = strchr(keys, '-');
> -        keyname_len = separator ? separator - keys : strlen(keys);
> +        separator = qemu_strchrnul(keys, '-');
> +        keyname_len = separator - keys;
>  
>          /* Be compatible with old interface, convert user inputted "<" */
>          if (keys[0] == '<' && keyname_len == 1) {
> @@ -2165,7 +2165,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>              keylist->value->u.qcode.data = idx;
>          }
>  
> -        if (!separator) {
> +        if (!*separator) {
>              break;
>          }
>          keys = separator + 1;
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 5721eff..828e8d6 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char *path, int flags,
>          assert(*path != '/');
>  
>          head = g_strdup(path);
> -        c = strchrnul(path, '/');
> +        c = qemu_strchrnul(path, '/');
>          if (*c) {
>              /* Intermediate path element */
>              head[c - path] = 0;
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index a663340..809090c 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -122,6 +122,14 @@ int qemu_strnlen(const char *s, int max_len);
>   * Returns: the pointer originally in @input.
>   */
>  char *qemu_strsep(char **input, const char *delim);
> +#ifdef CONFIG_STRCHRNUL
> +static inline const char *qemu_strchrnul(const char *s, int c)
> +{
> +    return strchrnul(s, c);
> +}
> +#else
> +const char *qemu_strchrnul(const char *s, int c);
> +#endif

Could we simply provide strchrnul() when it's missing?  Something like

 +#ifndef CONFIG_STRCHRNUL
 +const char *strchrnul(const char *s, int c);
 +#endif

in qemu/osdep.h.  Hmm, it looks like tacking qemu_ onto such things is
common practice.  Nevermind then.

>  time_t mktimegm(struct tm *tm);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
> diff --git a/monitor.c b/monitor.c
> index 6d0cec5..4484d74 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -797,9 +797,7 @@ static int compare_cmd(const char *name, const char *list)
>      p = list;
>      for(;;) {
>          pstart = p;
> -        p = strchr(p, '|');
> -        if (!p)
> -            p = pstart + strlen(pstart);
> +        p = qemu_strchrnul(p, '|');
>          if ((p - pstart) == len && !memcmp(pstart, name, len))
>              return 1;
>          if (*p == '\0')
> @@ -3400,9 +3398,7 @@ static void cmd_completion(Monitor *mon, const char *name, const char *list)
>      p = list;
>      for(;;) {
>          pstart = p;
> -        p = strchr(p, '|');
> -        if (!p)
> -            p = pstart + strlen(pstart);
> +        p = qemu_strchrnul(p, '|');
>          len = p - pstart;
>          if (len > sizeof(cmd) - 2)
>              len = sizeof(cmd) - 2;
> diff --git a/util/cutils.c b/util/cutils.c
> index 0de69e6..c365ddb 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -545,6 +545,21 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
>  }
>  
>  /**
> + * Searches for the first occurrence of 'c' in 's', and returns a pointer
> + * to the trailing null byte if none was found.
> + */
> +#ifndef CONFIG_STRCHRNUL

"If strchrnull is configured off, compile strchrnul" --- awkward.
#ifndef HAVE_STRCHRNUL would be nicer.

Should this be named HAVE_STRCHRNUL?  It's how it would be named with
Autoconf...

> +const char *qemu_strchrnul(const char *s, int c)
> +{
> +    const char *e = strchr(s, c);
> +    if (!e) {
> +        e = s + strlen(s);
> +    }
> +    return e;

Stupidest solution that could possibly work.  Okay :)

> +}
> +#endif
> +
> +/**
>   * parse_uint:
>   *
>   * @s: String to parse
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 58d1c23..54eca12 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -77,11 +77,7 @@ const char *get_opt_value(const char *p, char **value)
>  
>      *value = NULL;
>      while (1) {
> -        offset = strchr(p, ',');
> -        if (!offset) {
> -            offset = p + strlen(p);
> -        }
> -
> +        offset = qemu_strchrnul(p, ',');
>          length = offset - p;
>          if (*offset != '\0' && *(offset + 1) == ',') {
>              length++;
> diff --git a/util/uri.c b/util/uri.c
> index 8624a7a..8bdef84 100644
> --- a/util/uri.c
> +++ b/util/uri.c
> @@ -52,6 +52,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  
>  #include "qemu/uri.h"
>  
> @@ -2266,10 +2267,7 @@ struct QueryParams *query_params_parse(const char *query)
>          /* Find the next separator, or end of the string. */
>          end = strchr(query, '&');
>          if (!end) {
> -            end = strchr(query, ';');
> -        }
> -        if (!end) {
> -            end = query + strlen(query);
> +            end = qemu_strchrnul(query, ';');
>          }
>  
>          /* Find the first '=' character between here and end. */

How did you find the spots to convert to strchrnul()?

Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Peter Maydell 5 years, 9 months ago
On 11 June 2018 at 08:56, Markus Armbruster <armbru@redhat.com> wrote:
> You're not printing $strchrnul like we print other configuration
> results.  Hmm, we're not printing several of them.  Question for
> maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three
> coughed up by get_maintainer.pl): bug or feature?  If feature, how do we
> decide what to print?

If we printed everything that we tested for then the output would
be unhelpfully enormous. My view is that we should print the
"interesting" things for the user, ie the higher-level things
that the user could potentially turn on by installing more
libraries or has turned off explicitly or whatever. Reporting
whether the host OS has strchrnul or whether we've had to
provide our own implementation is doubly uninteresting:
 * there's nothing the user could do to change this
 * there is no visible effect (missing features, worse performance)

There's an argument that we should also log every config check
result somehow (I think autoconf configures do this), but I
don't think that our 'print stuff to stdout' is the right place
for that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Dr. David Alan Gilbert 5 years, 9 months ago
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 11 June 2018 at 08:56, Markus Armbruster <armbru@redhat.com> wrote:
> > You're not printing $strchrnul like we print other configuration
> > results.  Hmm, we're not printing several of them.  Question for
> > maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three
> > coughed up by get_maintainer.pl): bug or feature?  If feature, how do we
> > decide what to print?
> 
> If we printed everything that we tested for then the output would
> be unhelpfully enormous. My view is that we should print the
> "interesting" things for the user, ie the higher-level things
> that the user could potentially turn on by installing more
> libraries or has turned off explicitly or whatever. Reporting
> whether the host OS has strchrnul or whether we've had to
> provide our own implementation is doubly uninteresting:
>  * there's nothing the user could do to change this
>  * there is no visible effect (missing features, worse performance)
> 
> There's an argument that we should also log every config check
> result somehow (I think autoconf configures do this), but I
> don't think that our 'print stuff to stdout' is the right place
> for that.

We're not completely consistent, but I agree that we shouldn't print
the tiny things:

We're printing things that:
   a) Are user visible (e.g. KVM support)
   b) Reasonably major choices we make (e.g. coroutine backend)
   c) Some lesser things (madvise/posix_madvise/posix_memalign)
     However even (c) could be a problem if none were found

We probably shouldn't bother with printing things that have
no impact to either the user, or someone trying to cofnigure it and
wondering why a feature is missing.

Dave



> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Daniel P. Berrangé 5 years, 9 months ago
On Mon, Jun 11, 2018 at 09:52:55AM +0100, Peter Maydell wrote:
> On 11 June 2018 at 08:56, Markus Armbruster <armbru@redhat.com> wrote:
> > You're not printing $strchrnul like we print other configuration
> > results.  Hmm, we're not printing several of them.  Question for
> > maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three
> > coughed up by get_maintainer.pl): bug or feature?  If feature, how do we
> > decide what to print?
> 
> If we printed everything that we tested for then the output would
> be unhelpfully enormous. My view is that we should print the
> "interesting" things for the user, ie the higher-level things
> that the user could potentially turn on by installing more
> libraries or has turned off explicitly or whatever. Reporting
> whether the host OS has strchrnul or whether we've had to
> provide our own implementation is doubly uninteresting:
>  * there's nothing the user could do to change this
>  * there is no visible effect (missing features, worse performance)
> 
> There's an argument that we should also log every config check
> result somehow (I think autoconf configures do this), but I
> don't think that our 'print stuff to stdout' is the right place
> for that.

If you look at most non-trivial apps using autoconf, they have soo many
checks printed out, that they end up having to manually print out a
summary of the "important stuff" at the end so users can actually see
something they have a chance of reading.   QEMU's configure output
is essentially equivalent to this summary data, which is good.

It is sometimes useful to know the answer of individual checks, but
if we really wanted todo that, we should just create a logfile for
that info, or just write more into our existing config.log file.

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: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Peter Maydell 5 years, 9 months ago
On 11 June 2018 at 10:38, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Jun 11, 2018 at 09:52:55AM +0100, Peter Maydell wrote:
>> There's an argument that we should also log every config check
>> result somehow (I think autoconf configures do this), but I
>> don't think that our 'print stuff to stdout' is the right place
>> for that.

> It is sometimes useful to know the answer of individual checks, but
> if we really wanted todo that, we should just create a logfile for
> that info, or just write more into our existing config.log file.

Yes, that's what I had in mind. On the other hand, if we're not
in practice running into issues from users that would be easier
to debug with that detailed log info, it doesn't seem worth
the effort of trying to improve our config.log data...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Markus Armbruster 5 years, 9 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On 11 June 2018 at 08:56, Markus Armbruster <armbru@redhat.com> wrote:
>> You're not printing $strchrnul like we print other configuration
>> results.  Hmm, we're not printing several of them.  Question for
>> maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three
>> coughed up by get_maintainer.pl): bug or feature?  If feature, how do we
>> decide what to print?
>
> If we printed everything that we tested for then the output would
> be unhelpfully enormous. My view is that we should print the
> "interesting" things for the user, ie the higher-level things
> that the user could potentially turn on by installing more
> libraries or has turned off explicitly or whatever. Reporting
> whether the host OS has strchrnul or whether we've had to
> provide our own implementation is doubly uninteresting:
>  * there's nothing the user could do to change this
>  * there is no visible effect (missing features, worse performance)

Care to clean out out existing "uninteresting" prints?

> There's an argument that we should also log every config check
> result somehow (I think autoconf configures do this), but I
> don't think that our 'print stuff to stdout' is the right place
> for that.

Makes sense.  Volunteers?

Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Keno Fischer 5 years, 9 months ago
> Suggest return strchrnul("Hello World", 'W') != 6, to avoid worries
> about a sufficiently smart compilers optimizing out a call that would
> otherwise fail to link, say because headers don't match libraries.

I'm happy to do that, but then again, a sufficiently smart compiler might
constant fold this call entirely, so to be completely safe maybe we need

extern char *haystack;
extern char needle;
int main(void) { return strchrnul(haystack, needle) != 6; }

Though frankly if you're in a position for this to be a problem, you've
got bigger problems. Happy to change this though.

> Should this be named HAVE_STRCHRNUL?  It's how it would be named with
> Autoconf...

Ok, I will rename this.

>> +const char *qemu_strchrnul(const char *s, int c)
>> +{
>> +    const char *e = strchr(s, c);
>> +    if (!e) {
>> +        e = s + strlen(s);
>> +    }
>> +    return e;
>
> Stupidest solution that could possibly work.  Okay :)

Well, it's the pattern that was used everywhere in place of this function,
so certainly from a commit factoring this seemed like the most sensible
thing to do.

> How did you find the spots to convert to strchrnul()?

I audited uses of `strchr` and checked for whether they were really doing
`strchrnul` (plus the one use case in code that used to only ever be compiled
on Linux).

Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Posted by Markus Armbruster 5 years, 9 months ago
Keno Fischer <keno@juliacomputing.com> writes:

>> Suggest return strchrnul("Hello World", 'W') != 6, to avoid worries
>> about a sufficiently smart compilers optimizing out a call that would
>> otherwise fail to link, say because headers don't match libraries.
>
> I'm happy to do that, but then again, a sufficiently smart compiler might
> constant fold this call entirely, so to be completely safe maybe we need
>
> extern char *haystack;
> extern char needle;
> int main(void) { return strchrnul(haystack, needle) != 6; }
>
> Though frankly if you're in a position for this to be a problem, you've
> got bigger problems. Happy to change this though.

Yes, please.  You even get to pick your favorite number for the right
hand side of the comparison ;)

>> Should this be named HAVE_STRCHRNUL?  It's how it would be named with
>> Autoconf...
>
> Ok, I will rename this.
>
>>> +const char *qemu_strchrnul(const char *s, int c)
>>> +{
>>> +    const char *e = strchr(s, c);
>>> +    if (!e) {
>>> +        e = s + strlen(s);
>>> +    }
>>> +    return e;
>>
>> Stupidest solution that could possibly work.  Okay :)
>
> Well, it's the pattern that was used everywhere in place of this function,
> so certainly from a commit factoring this seemed like the most sensible
> thing to do.
>
>> How did you find the spots to convert to strchrnul()?
>
> I audited uses of `strchr` and checked for whether they were really doing
> `strchrnul` (plus the one use case in code that used to only ever be compiled
> on Linux).

Sounds good.  Thanks!