Re: [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul

2018-06-01 Thread Eric Blake 

On 05/31/2018 08:25 PM, Keno Fischer wrote:

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




+++ b/util/cutils.c
@@ -545,6 +545,19 @@ 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.
+ */
+const char *qemu_strchrnul(const char *s, int c)
+{
+const char *e = strchr(s, c);
+if (!e) {
+e = s + strlen(s);
+}
+return e;
+}


This is twice as slow on glibc systems when the pointer to NUL is 
returned (because it has to traverse the string twice); it's better to 
have a configure check for whether strchrnul exists, and if so, use that 
directly.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul

2018-06-01 Thread Dr. David Alan Gilbert 
* Greg Kurz (gr...@kaod.org) wrote:
> On Thu, 31 May 2018 21:25:56 -0400
> Keno Fischer  wrote:
> 
> > 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 
> > ---
> > 
> 
> And possibly we could detect in configure if the host has strchrnul() and
> use it, but this optimization can be done later.
> 
> I haven't checked if there could be other candidates in the current code
> base though. Also, this patch touches some other subsystems, so I'm Cc'ing
> to the other maintainers as reported by ./scripts/get_maintainer.pl:

That looks fine from my point of view;  I can see you could probably
also use it in the code at the start of the while loop in hmp_sendkey:

while (1) {
separator = strchr(keys, '-');
keyname_len = separator ? separator - keys : strlen(keys);

Dave

> Greg Kurz  (supporter:virtio-9p)
> Markus Armbruster  (supporter:QMP)
> "Dr. David Alan Gilbert"  (maintainer:Human Monitor 
> (HMP))
> qemu-devel@nongnu.org (open list:All patches CC here)
> 
> Anyway,
> 
> Acked-by: Greg Kurz 
> 
> > Changes since v1: New patch
> > 
> >  hw/9pfs/9p-local.c|  2 +-
> >  include/qemu/cutils.h |  1 +
> >  monitor.c |  8 ++--
> >  util/cutils.c | 13 +
> >  util/qemu-option.c|  6 +-
> >  util/uri.c|  6 ++
> >  6 files changed, 20 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index b37b1db..bcf2798 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..bc40c30 100644
> > --- a/include/qemu/cutils.h
> > +++ b/include/qemu/cutils.h
> > @@ -122,6 +122,7 @@ int qemu_strnlen(const char *s, int max_len);
> >   * Returns: the pointer originally in @input.
> >   */
> >  char *qemu_strsep(char **input, const char *delim);
> > +const char *qemu_strchrnul(const char *s, int c);
> >  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 922cfc0..e1f01c4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -798,9 +798,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')
> > @@ -3401,9 +3399,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..6e078b0 100644
> > --- a/util/cutils.c
> > +++ b/util/cutils.c
> > @@ -545,6 +545,19 @@ 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.
> > + */
> > +const char *qemu_strchrnul(const char *s, int c)
> > +{
> > +const char *e = strchr(s, c);
> > +if (!e) {
> > +e = s + strlen(s);
> > +}
> > +return e;
> > +}
> > +
> > +/**
> >   * 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
> >

Re: [Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul

2018-06-01 Thread Greg Kurz 
On Thu, 31 May 2018 21:25:56 -0400
Keno Fischer  wrote:

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

And possibly we could detect in configure if the host has strchrnul() and
use it, but this optimization can be done later.

I haven't checked if there could be other candidates in the current code
base though. Also, this patch touches some other subsystems, so I'm Cc'ing
to the other maintainers as reported by ./scripts/get_maintainer.pl:

Greg Kurz  (supporter:virtio-9p)
Markus Armbruster  (supporter:QMP)
"Dr. David Alan Gilbert"  (maintainer:Human Monitor (HMP))
qemu-devel@nongnu.org (open list:All patches CC here)

Anyway,

Acked-by: Greg Kurz 

> Changes since v1: New patch
> 
>  hw/9pfs/9p-local.c|  2 +-
>  include/qemu/cutils.h |  1 +
>  monitor.c |  8 ++--
>  util/cutils.c | 13 +
>  util/qemu-option.c|  6 +-
>  util/uri.c|  6 ++
>  6 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index b37b1db..bcf2798 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..bc40c30 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -122,6 +122,7 @@ int qemu_strnlen(const char *s, int max_len);
>   * Returns: the pointer originally in @input.
>   */
>  char *qemu_strsep(char **input, const char *delim);
> +const char *qemu_strchrnul(const char *s, int c);
>  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 922cfc0..e1f01c4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -798,9 +798,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')
> @@ -3401,9 +3399,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..6e078b0 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -545,6 +545,19 @@ 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.
> + */
> +const char *qemu_strchrnul(const char *s, int c)
> +{
> +const char *e = strchr(s, c);
> +if (!e) {
> +e = s + strlen(s);
> +}
> +return e;
> +}
> +
> +/**
>   * 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 '='

[Qemu-devel] [PATCH v2 01/20] cutils: Provide strchrnul

2018-05-31 Thread Keno Fischer 
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 
---

Changes since v1: New patch

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

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b37b1db..bcf2798 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..bc40c30 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -122,6 +122,7 @@ int qemu_strnlen(const char *s, int max_len);
  * Returns: the pointer originally in @input.
  */
 char *qemu_strsep(char **input, const char *delim);
+const char *qemu_strchrnul(const char *s, int c);
 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 922cfc0..e1f01c4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -798,9 +798,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')
@@ -3401,9 +3399,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..6e078b0 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -545,6 +545,19 @@ 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.
+ */
+const char *qemu_strchrnul(const char *s, int c)
+{
+const char *e = strchr(s, c);
+if (!e) {
+e = s + strlen(s);
+}
+return e;
+}
+
+/**
  * 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