[Qemu-devel] [PATCH v3 0/5] Prepratory cleanup for 9p darwin support

[Qemu-devel] [PATCH v3 0/5] Prepratory cleanup for 9p darwin support

[Keno Fischer]

Hi Greg,

this is a respin of just the first couple of patches of my 9p Darwin
series. These patches should be applicable independent of the darwin
work.

Keno Fischer (5):
  cutils: Provide strchrnul
  9p: xattr: Fix crashes due to free of uninitialized value
  9p: local: Avoid warning if FS_IOC_GETVERSION is not defined
  9p: Properly error check and translate flags in unlinkat
  9p: xattr: Properly translate xattrcreate flags

 configure             | 15 +++++++++++++++
 hmp.c                 |  8 ++++----
 hw/9pfs/9p-handle.c   |  8 +-------
 hw/9pfs/9p-local.c    | 42 ++++++++++++++++++++++++------------------
 hw/9pfs/9p.c          | 34 ++++++++++++++++++++++++++++------
 hw/9pfs/9p.h          |  4 ++++
 include/qemu/cutils.h |  8 ++++++++
 monitor.c             |  8 ++------
 util/cutils.c         | 15 +++++++++++++++
 util/qemu-option.c    |  6 +-----
 util/uri.c            |  6 ++----
 11 files changed, 104 insertions(+), 50 deletions(-)

-- 
2.8.1

[Qemu-devel] [PATCH v3 1/5] cutils: Provide strchrnul

[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 <keno@juliacomputing.com>
Acked-by: Greg Kurz <groug@kaod.org>
---

Changes since v2:
 * Add configure check as suggested by Greg Kurz <groug@kaod.org>,
   and requested by Eric Blake <eblake@redhat.com>
 * Use qemu_strchrnul in hmp_sendkey as suggested by
   Dr. David Alan Gilbert <dgilbert@redhat.com>

 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 a6a4616..1b3ca4e 100755
--- a/configure
+++ b/configure
@@ -4754,6 +4754,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'); }
+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
@@ -6210,6 +6222,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 7758c38..304ef72 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 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..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 v3 1/5] cutils: Provide strchrnul

[Greg Kurz]

On Sat,  2 Jun 2018 17:29:35 -0400
Keno Fischer <keno@juliacomputing.com> 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 <keno@juliacomputing.com>
> Acked-by: Greg Kurz <groug@kaod.org>
> ---
> 
> Changes since v2:
>  * Add configure check as suggested by Greg Kurz <groug@kaod.org>,
>    and requested by Eric Blake <eblake@redhat.com>
>  * Use qemu_strchrnul in hmp_sendkey as suggested by
>    Dr. David Alan Gilbert <dgilbert@redhat.com>
> 

My Acked-by stands with the new changes.

If everyone is okay, maybe better for this to go through my tree
since it is required for the "9p: Add support for Darwin" 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(-)
> 
> diff --git a/configure b/configure
> index a6a4616..1b3ca4e 100755
> --- a/configure
> +++ b/configure
> @@ -4754,6 +4754,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'); }
> +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
> @@ -6210,6 +6222,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 7758c38..304ef72 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 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..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. */

Re: [Qemu-devel] [PATCH v3 1/5] cutils: Provide strchrnul

[Greg Kurz]

On Tue, 5 Jun 2018 13:48:02 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Sat,  2 Jun 2018 17:29:35 -0400
> Keno Fischer <keno@juliacomputing.com> 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 <keno@juliacomputing.com>
> > Acked-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > Changes since v2:
> >  * Add configure check as suggested by Greg Kurz <groug@kaod.org>,
> >    and requested by Eric Blake <eblake@redhat.com>
> >  * Use qemu_strchrnul in hmp_sendkey as suggested by
> >    Dr. David Alan Gilbert <dgilbert@redhat.com>
> >   
> 
> My Acked-by stands with the new changes.
> 

But this doesn't build on CentOS 7.5.1804 (gcc 4.8.5 20150623 (Red Hat 4.8.5-28))

ERROR: configure test passed without -Werror but failed with -Werror.
       This is probably a bug in the configure script. The failing command
       will be at the bottom of config.log.
       You can run configure with --disable-werror to bypass this check.

More below...

> If everyone is okay, maybe better for this to go through my tree
> since it is required for the "9p: Add support for Darwin" 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(-)
> > 
> > diff --git a/configure b/configure
> > index a6a4616..1b3ca4e 100755
> > --- a/configure
> > +++ b/configure
> > @@ -4754,6 +4754,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'); }

config-temp/qemu-conf.c: In function ‘main’:
config-temp/qemu-conf.c:2:1: error: control reaches end of non-void
 function [-Werror=return-type]
 int main(void) { (void)strchrnul("Hello World", 'W'); }
 ^
cc1: all warnings being treated as errors

Adding 'return 0;' like for syslog a few lines above is enough.

> > +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
> > @@ -6210,6 +6222,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 7758c38..304ef72 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 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..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. */  
> 

[Qemu-devel] [PATCH v3 2/5] 9p: xattr: Fix crashes due to free of uninitialized value

[Keno Fischer]

If the size returned from llistxattr/lgetxattr is 0, we skipped
the malloc call, leaving xattr.value uninitialized. However, this
value is later passed to `g_free` without any further checks,
causing an error. Fix that by always calling g_malloc unconditionally.
If `size` is 0, it will return NULL, which is safe to pass to g_free.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---

Changes since v2:
 * Fix another instance of the problematic pattern later in the same function.

 hw/9pfs/9p.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d74302d..4386d69 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3256,8 +3256,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
         xattr_fidp->fs.xattr.len = size;
         xattr_fidp->fid_type = P9_FID_XATTR;
         xattr_fidp->fs.xattr.xattrwalk_fid = true;
+        xattr_fidp->fs.xattr.value = g_malloc0(size);
         if (size) {
-            xattr_fidp->fs.xattr.value = g_malloc0(size);
             err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
                                      xattr_fidp->fs.xattr.value,
                                      xattr_fidp->fs.xattr.len);
@@ -3289,8 +3289,8 @@ static void coroutine_fn v9fs_xattrwalk(void *opaque)
         xattr_fidp->fs.xattr.len = size;
         xattr_fidp->fid_type = P9_FID_XATTR;
         xattr_fidp->fs.xattr.xattrwalk_fid = true;
+        xattr_fidp->fs.xattr.value = g_malloc0(size);
         if (size) {
-            xattr_fidp->fs.xattr.value = g_malloc0(size);
             err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
                                     &name, xattr_fidp->fs.xattr.value,
                                     xattr_fidp->fs.xattr.len);
-- 
2.8.1

[Qemu-devel] [PATCH v3 3/5] 9p: local: Avoid warning if FS_IOC_GETVERSION is not defined

[Keno Fischer]

Both `stbuf` and `local_ioc_getversion` where unused when
FS_IOC_GETVERSION was not defined, causing a compiler warning.

Reorganize the code to avoid this warning.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 hw/9pfs/9p-local.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 304ef72..828e8d6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1373,10 +1373,10 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir,
     return ret;
 }
 
+#ifdef FS_IOC_GETVERSION
 static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
                                 mode_t st_mode, uint64_t *st_gen)
 {
-#ifdef FS_IOC_GETVERSION
     int err;
     V9fsFidOpenState fid_open;
 
@@ -1395,32 +1395,21 @@ static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
     err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
     local_close(ctx, &fid_open);
     return err;
-#else
-    errno = ENOTTY;
-    return -1;
-#endif
 }
+#endif
 
-static int local_init(FsContext *ctx, Error **errp)
+static int local_ioc_getversion_init(FsContext *ctx, LocalData *data, Error **errp)
 {
+#ifdef FS_IOC_GETVERSION
     struct statfs stbuf;
-    LocalData *data = g_malloc(sizeof(*data));
 
-    data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
-    if (data->mountfd == -1) {
-        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
-        goto err;
-    }
-
-#ifdef FS_IOC_GETVERSION
     /*
      * use ioc_getversion only if the ioctl is definied
      */
     if (fstatfs(data->mountfd, &stbuf) < 0) {
-        close_preserve_errno(data->mountfd);
         error_setg_errno(errp, errno,
-            "failed to stat file system at '%s'", ctx->fs_root);
-        goto err;
+                         "failed to stat file system at '%s'", ctx->fs_root);
+        return -1;
     }
     switch (stbuf.f_type) {
     case EXT2_SUPER_MAGIC:
@@ -1431,6 +1420,23 @@ static int local_init(FsContext *ctx, Error **errp)
         break;
     }
 #endif
+    return 0;
+}
+
+static int local_init(FsContext *ctx, Error **errp)
+{
+    LocalData *data = g_malloc(sizeof(*data));
+
+    data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
+    if (data->mountfd == -1) {
+        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
+        goto err;
+    }
+
+    if (local_ioc_getversion_init(ctx, data, errp) < 0) {
+        close(data->mountfd);
+        goto err;
+    }
 
     if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
         ctx->xops = passthrough_xattr_ops;
-- 
2.8.1

[Qemu-devel] [PATCH v3 4/5] 9p: Properly check/translate flags in unlinkat

[Keno Fischer]

The 9p-local code previously relied on P9_DOTL_AT_REMOVEDIR and AT_REMOVEDIR
having the same numerical value and deferred any errorchecking to the
syscall itself. However, while the former assumption is true on Linux,
it is not true in general. 9p-handle did this properly however. Move
the translation code to the generic 9p server code and add an error
if unrecognized flags are passed.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---

Changes since v2:
 * Remove 9p-handle code that did the same translation and is now incorrect.

 hw/9pfs/9p-handle.c |  8 +-------
 hw/9pfs/9p.c        | 13 +++++++++++--
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index 4dc0d2b..f3641db 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -559,19 +559,13 @@ static int handle_unlinkat(FsContext *ctx, V9fsPath *dir,
 {
     int dirfd, ret;
     HandleData *data = (HandleData *) ctx->private;
-    int rflags;
 
     dirfd = open_by_handle(data->mountfd, dir->data, O_PATH);
     if (dirfd < 0) {
         return dirfd;
     }
 
-    rflags = 0;
-    if (flags & P9_DOTL_AT_REMOVEDIR) {
-        rflags |= AT_REMOVEDIR;
-    }
-
-    ret = unlinkat(dirfd, name, rflags);
+    ret = unlinkat(dirfd, name, flags);
 
     close(dirfd);
     return ret;
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4386d69..c842ec5 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2522,7 +2522,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
 {
     int err = 0;
     V9fsString name;
-    int32_t dfid, flags;
+    int32_t dfid, flags, rflags = 0;
     size_t offset = 7;
     V9fsPath path;
     V9fsFidState *dfidp;
@@ -2549,6 +2549,15 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
         goto out_nofid;
     }
 
+    if (flags & ~P9_DOTL_AT_REMOVEDIR) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
+    if (flags & P9_DOTL_AT_REMOVEDIR) {
+        rflags |= AT_REMOVEDIR;
+    }
+
     dfidp = get_fid(pdu, dfid);
     if (dfidp == NULL) {
         err = -EINVAL;
@@ -2567,7 +2576,7 @@ static void coroutine_fn v9fs_unlinkat(void *opaque)
     if (err < 0) {
         goto out_err;
     }
-    err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, flags);
+    err = v9fs_co_unlinkat(pdu, &dfidp->path, &name, rflags);
     if (!err) {
         err = offset;
     }
-- 
2.8.1

[Qemu-devel] [PATCH v3 5/5] 9p: xattr: Properly translate xattrcreate flags

[Keno Fischer]

As with unlinkat, these flags come from the client and need to
be translated to their host values. The protocol values happen
to match linux, but that need not be true in general.

Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---

Changes since v2: New patch

 hw/9pfs/9p.c | 17 +++++++++++++++--
 hw/9pfs/9p.h |  4 ++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index c842ec5..eef289e 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3327,7 +3327,7 @@ out_nofid:
 
 static void coroutine_fn v9fs_xattrcreate(void *opaque)
 {
-    int flags;
+    int flags, rflags = 0;
     int32_t fid;
     uint64_t size;
     ssize_t err = 0;
@@ -3344,6 +3344,19 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
     }
     trace_v9fs_xattrcreate(pdu->tag, pdu->id, fid, name.data, size, flags);
 
+    if (flags & ~(P9_XATTR_CREATE | P9_XATTR_REPLACE)) {
+        err = -EINVAL;
+        goto out_nofid;
+    }
+
+    if (flags & P9_XATTR_CREATE) {
+        rflags |= XATTR_CREATE;
+    }
+
+    if (flags & P9_XATTR_REPLACE) {
+        rflags |= XATTR_REPLACE;
+    }
+
     if (size > XATTR_SIZE_MAX) {
         err = -E2BIG;
         goto out_nofid;
@@ -3365,7 +3378,7 @@ static void coroutine_fn v9fs_xattrcreate(void *opaque)
     xattr_fidp->fs.xattr.copied_len = 0;
     xattr_fidp->fs.xattr.xattrwalk_fid = false;
     xattr_fidp->fs.xattr.len = size;
-    xattr_fidp->fs.xattr.flags = flags;
+    xattr_fidp->fs.xattr.flags = rflags;
     v9fs_string_init(&xattr_fidp->fs.xattr.name);
     v9fs_string_copy(&xattr_fidp->fs.xattr.name, &name);
     xattr_fidp->fs.xattr.value = g_malloc0(size);
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index bad8ee7..6081b0d 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -169,6 +169,10 @@ typedef struct V9fsConf
     char *fsdev_id;
 } V9fsConf;
 
+/* 9p2000.L xattr flags (matches Linux values) */
+#define P9_XATTR_CREATE 1
+#define P9_XATTR_REPLACE 2
+
 typedef struct V9fsXattr
 {
     uint64_t copied_len;
-- 
2.8.1

Re: [Qemu-devel] [PATCH v3 0/5] Prepratory cleanup for 9p darwin support

[Greg Kurz]

On Sat,  2 Jun 2018 17:29:34 -0400
Keno Fischer <keno@juliacomputing.com> wrote:

> Hi Greg,
> 
> this is a respin of just the first couple of patches of my 9p Darwin
> series. These patches should be applicable independent of the darwin
> work.
> 

I've applied patches 2 to 5 for the time being.

> Keno Fischer (5):
>   cutils: Provide strchrnul
>   9p: xattr: Fix crashes due to free of uninitialized value
>   9p: local: Avoid warning if FS_IOC_GETVERSION is not defined
>   9p: Properly error check and translate flags in unlinkat
>   9p: xattr: Properly translate xattrcreate flags
> 
>  configure             | 15 +++++++++++++++
>  hmp.c                 |  8 ++++----
>  hw/9pfs/9p-handle.c   |  8 +-------
>  hw/9pfs/9p-local.c    | 42 ++++++++++++++++++++++++------------------
>  hw/9pfs/9p.c          | 34 ++++++++++++++++++++++++++++------
>  hw/9pfs/9p.h          |  4 ++++
>  include/qemu/cutils.h |  8 ++++++++
>  monitor.c             |  8 ++------
>  util/cutils.c         | 15 +++++++++++++++
>  util/qemu-option.c    |  6 +-----
>  util/uri.c            |  6 ++----
>  11 files changed, 104 insertions(+), 50 deletions(-)
>