[Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Greg Kurz]

The readdir_r() function has a broken design and should not be used anymore.
It is expected to be obsoleted in a future version of POSIX.1:

http://austingroupbugs.net/view.php?id=696#c2857

Glibc has already announced that 2.24 (scheduled for August 2016) will
deprecates readdir_r() and encourages people to use readdir() with
external synchronization instead.

This series replaces readdir_r() by readdir() and changes the internal API
in fsdev/file-op-9p.h to be readdir-like, like it was before commit
"5f524c1ebcc5 use readdir_r instead of readdir for reentrancy".

The 9p code runs in coroutines, called either:
- from the QEMU main-loop when ioventfd=on, which is the default, or
- from vCPU context when ioeventfd=off, with the qemu_global_mutex
  taken

All hypothetical critical sections around the use of readdir() are then
already serialized. This series introduces a serialization anyway for the
code to be fully independant from the context.

Since POSIX.1 will require readdir() to be thread-safe when employed on
different directory streams, and glibc already does that, the choice
was made to have per-directory locking.

Unsurprisingly, there is no contention and the locking/unlocking hasn't
any noticeable impact on performance.

Since early glibc-2.24 users are already encountering build breaks, I guess
this should go upstream sooner than later. I plan to issue a pull request
next week, or even before if possible.

Please comment !

Thanks.

---

Greg Kurz (4):
      9p: drop useless out: label
      9p: introduce the V9fsDir type
      9p: add locking to V9fsDir
      9p: switch back to readdir()


 fsdev/file-op-9p.h  |    3 +-
 hw/9pfs/9p-handle.c |   24 +++++++++-----------
 hw/9pfs/9p-local.c  |   36 ++++++++++++++++--------------
 hw/9pfs/9p-proxy.c  |   26 ++++++++++-----------
 hw/9pfs/9p-synth.c  |   23 ++++++++-----------
 hw/9pfs/9p-synth.h  |    1 +
 hw/9pfs/9p.c        |   62 ++++++++++++++++++++++++++++++---------------------
 hw/9pfs/9p.h        |   22 +++++++++++++++++-
 hw/9pfs/codir.c     |   12 ++++++----
 hw/9pfs/coth.h      |    3 +-
 10 files changed, 120 insertions(+), 92 deletions(-)

--
Greg

[Qemu-devel] [PATCH 1/4] 9p: drop useless out: label

[Greg Kurz]

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/9pfs/9p.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 587e901f81cc..12bd688f37d3 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1646,15 +1646,15 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         }
         err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
         if (err < 0) {
-            goto out;
+            break;
         }
         err = v9fs_co_lstat(pdu, &path, &stbuf);
         if (err < 0) {
-            goto out;
+            break;
         }
         err = stat_to_v9stat(pdu, &path, &stbuf, &v9stat);
         if (err < 0) {
-            goto out;
+            break;
         }
         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "S", &v9stat);
@@ -1671,7 +1671,7 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         v9fs_path_free(&path);
         saved_dir_pos = dent->d_off;
     }
-out:
+
     g_free(dent);
     v9fs_path_free(&path);
     if (err < 0) {

[Qemu-devel] [PATCH 2/4] 9p: introduce the V9fsDir type

[Greg Kurz]

If we are to switch back to readdir(), we need a more complex type than
DIR * to be able to serialize concurrent accesses to the directory stream.

This patch introduces a placeholder type and fixes all users.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/9pfs/9p-handle.c |   18 +++++++++---------
 hw/9pfs/9p-local.c  |   18 +++++++++---------
 hw/9pfs/9p-proxy.c  |   20 ++++++++++----------
 hw/9pfs/9p.c        |   12 ++++++------
 hw/9pfs/9p.h        |    6 +++++-
 5 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index 894041488abb..c5df05300700 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -112,7 +112,7 @@ static int handle_close(FsContext *ctx, V9fsFidOpenState *fs)
 
 static int handle_closedir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return closedir(fs->dir);
+    return closedir(fs->dir.stream);
 }
 
 static int handle_open(FsContext *ctx, V9fsPath *fs_path,
@@ -132,8 +132,8 @@ static int handle_opendir(FsContext *ctx,
     if (ret < 0) {
         return -1;
     }
-    fs->dir = fdopendir(ret);
-    if (!fs->dir) {
+    fs->dir.stream = fdopendir(ret);
+    if (!fs->dir.stream) {
         return -1;
     }
     return 0;
@@ -141,24 +141,24 @@ static int handle_opendir(FsContext *ctx,
 
 static void handle_rewinddir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    rewinddir(fs->dir);
+    rewinddir(fs->dir.stream);
 }
 
 static off_t handle_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return telldir(fs->dir);
+    return telldir(fs->dir.stream);
 }
 
 static int handle_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
                             struct dirent *entry,
                             struct dirent **result)
 {
-    return readdir_r(fs->dir, entry, result);
+    return readdir_r(fs->dir.stream, entry, result);
 }
 
 static void handle_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
 {
-    seekdir(fs->dir, off);
+    seekdir(fs->dir.stream, off);
 }
 
 static ssize_t handle_preadv(FsContext *ctx, V9fsFidOpenState *fs,
@@ -262,7 +262,7 @@ static int handle_fstat(FsContext *fs_ctx, int fid_type,
     int fd;
 
     if (fid_type == P9_FID_DIR) {
-        fd = dirfd(fs->dir);
+        fd = dirfd(fs->dir.stream);
     } else {
         fd = fs->fd;
     }
@@ -409,7 +409,7 @@ static int handle_fsync(FsContext *ctx, int fid_type,
     int fd;
 
     if (fid_type == P9_FID_DIR) {
-        fd = dirfd(fs->dir);
+        fd = dirfd(fs->dir.stream);
     } else {
         fd = fs->fd;
     }
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 16f45f4854ca..3281acdef164 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -348,7 +348,7 @@ static int local_close(FsContext *ctx, V9fsFidOpenState *fs)
 
 static int local_closedir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return closedir(fs->dir);
+    return closedir(fs->dir.stream);
 }
 
 static int local_open(FsContext *ctx, V9fsPath *fs_path,
@@ -370,9 +370,9 @@ static int local_opendir(FsContext *ctx,
     char *path = fs_path->data;
 
     buffer = rpath(ctx, path);
-    fs->dir = opendir(buffer);
+    fs->dir.stream = opendir(buffer);
     g_free(buffer);
-    if (!fs->dir) {
+    if (!fs->dir.stream) {
         return -1;
     }
     return 0;
@@ -380,12 +380,12 @@ static int local_opendir(FsContext *ctx,
 
 static void local_rewinddir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    rewinddir(fs->dir);
+    rewinddir(fs->dir.stream);
 }
 
 static off_t local_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return telldir(fs->dir);
+    return telldir(fs->dir.stream);
 }
 
 static int local_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
@@ -395,7 +395,7 @@ static int local_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
     int ret;
 
 again:
-    ret = readdir_r(fs->dir, entry, result);
+    ret = readdir_r(fs->dir.stream, entry, result);
     if (ctx->export_flags & V9FS_SM_MAPPED) {
         entry->d_type = DT_UNKNOWN;
     } else if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
@@ -411,7 +411,7 @@ again:
 
 static void local_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
 {
-    seekdir(fs->dir, off);
+    seekdir(fs->dir.stream, off);
 }
 
 static ssize_t local_preadv(FsContext *ctx, V9fsFidOpenState *fs,
@@ -610,7 +610,7 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
     int err, fd;
 
     if (fid_type == P9_FID_DIR) {
-        fd = dirfd(fs->dir);
+        fd = dirfd(fs->dir.stream);
     } else {
         fd = fs->fd;
     }
@@ -998,7 +998,7 @@ static int local_fsync(FsContext *ctx, int fid_type,
     int fd;
 
     if (fid_type == P9_FID_DIR) {
-        fd = dirfd(fs->dir);
+        fd = dirfd(fs->dir.stream);
     } else {
         fd = fs->fd;
     }
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 00a4eb2a7bcf..63b074781589 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -633,7 +633,7 @@ static int proxy_close(FsContext *ctx, V9fsFidOpenState *fs)
 
 static int proxy_closedir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return closedir(fs->dir);
+    return closedir(fs->dir.stream);
 }
 
 static int proxy_open(FsContext *ctx, V9fsPath *fs_path,
@@ -652,14 +652,14 @@ static int proxy_opendir(FsContext *ctx,
 {
     int serrno, fd;
 
-    fs->dir = NULL;
+    fs->dir.stream = NULL;
     fd = v9fs_request(ctx->private, T_OPEN, NULL, "sd", fs_path, O_DIRECTORY);
     if (fd < 0) {
         errno = -fd;
         return -1;
     }
-    fs->dir = fdopendir(fd);
-    if (!fs->dir) {
+    fs->dir.stream = fdopendir(fd);
+    if (!fs->dir.stream) {
         serrno = errno;
         close(fd);
         errno = serrno;
@@ -670,24 +670,24 @@ static int proxy_opendir(FsContext *ctx,
 
 static void proxy_rewinddir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    rewinddir(fs->dir);
+    rewinddir(fs->dir.stream);
 }
 
 static off_t proxy_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return telldir(fs->dir);
+    return telldir(fs->dir.stream);
 }
 
 static int proxy_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
                            struct dirent *entry,
                            struct dirent **result)
 {
-    return readdir_r(fs->dir, entry, result);
+    return readdir_r(fs->dir.stream, entry, result);
 }
 
 static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
 {
-    seekdir(fs->dir, off);
+    seekdir(fs->dir.stream, off);
 }
 
 static ssize_t proxy_preadv(FsContext *ctx, V9fsFidOpenState *fs,
@@ -791,7 +791,7 @@ static int proxy_fstat(FsContext *fs_ctx, int fid_type,
     int fd;
 
     if (fid_type == P9_FID_DIR) {
-        fd = dirfd(fs->dir);
+        fd = dirfd(fs->dir.stream);
     } else {
         fd = fs->fd;
     }
@@ -936,7 +936,7 @@ static int proxy_fsync(FsContext *ctx, int fid_type,
     int fd;
 
     if (fid_type == P9_FID_DIR) {
-        fd = dirfd(fs->dir);
+        fd = dirfd(fs->dir.stream);
     } else {
         fd = fs->fd;
     }
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 12bd688f37d3..451ecbde0b08 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -231,7 +231,7 @@ static int v9fs_reopen_fid(V9fsPDU *pdu, V9fsFidState *f)
             } while (err == -EINTR && !pdu->cancelled);
         }
     } else if (f->fid_type == P9_FID_DIR) {
-        if (f->fs.dir == NULL) {
+        if (f->fs.dir.stream == NULL) {
             do {
                 err = v9fs_co_opendir(pdu, f);
             } while (err == -EINTR && !pdu->cancelled);
@@ -345,7 +345,7 @@ static int free_fid(V9fsPDU *pdu, V9fsFidState *fidp)
             retval = v9fs_co_close(pdu, &fidp->fs);
         }
     } else if (fidp->fid_type == P9_FID_DIR) {
-        if (fidp->fs.dir != NULL) {
+        if (fidp->fs.dir.stream != NULL) {
             retval = v9fs_co_closedir(pdu, &fidp->fs);
         }
     } else if (fidp->fid_type == P9_FID_XATTR) {
@@ -443,7 +443,7 @@ void v9fs_reclaim_fd(V9fsPDU *pdu)
                 reclaim_count++;
             }
         } else if (f->fid_type == P9_FID_DIR) {
-            if (f->fs.dir != NULL) {
+            if (f->fs.dir.stream != NULL) {
                 /*
                  * Up the reference count so that
                  * a clunk request won't free this fid
@@ -451,8 +451,8 @@ void v9fs_reclaim_fd(V9fsPDU *pdu)
                 f->ref++;
                 f->rclm_lst = reclaim_list;
                 reclaim_list = f;
-                f->fs_reclaim.dir = f->fs.dir;
-                f->fs.dir = NULL;
+                f->fs_reclaim.dir.stream = f->fs.dir.stream;
+                f->fs.dir.stream = NULL;
                 reclaim_count++;
             }
         }
@@ -1887,7 +1887,7 @@ static void v9fs_readdir(void *opaque)
         retval = -EINVAL;
         goto out_nofid;
     }
-    if (!fidp->fs.dir) {
+    if (!fidp->fs.dir.stream) {
         retval = -EINVAL;
         goto out;
     }
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 43378943cdda..92ee309ef4ba 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -167,13 +167,17 @@ typedef struct V9fsXattr
     int flags;
 } V9fsXattr;
 
+typedef struct V9fsDir {
+    DIR *stream;
+} V9fsDir;
+
 /*
  * Filled by fs driver on open and other
  * calls.
  */
 union V9fsFidOpenState {
     int fd;
-    DIR *dir;
+    V9fsDir dir;
     V9fsXattr xattr;
     /*
      * private pointer for fs drivers, that

[Qemu-devel] [PATCH 3/4] 9p: add locking to V9fsDir

[Greg Kurz]

If several threads call concurrently readdir() with the same directory
stream pointer, it is possible that they all get a pointer to the same
dirent structure, whose content is overwritten each time readdir() is
called.

We must thus serialize accesses to the dirent structure.

This may be achieved with a mutex like below:

lock_mutex();

readdir();

// work with the dirent

unlock_mutex();

This patch adds all the locking, to prepare the switch to readdir().

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/9pfs/9p.c |   21 +++++++++++++++++++++
 hw/9pfs/9p.h |   16 ++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 451ecbde0b08..3ed077b25b98 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -300,6 +300,9 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
     f->next = s->fid_list;
     s->fid_list = f;
 
+    v9fs_readdir_init(&f->fs.dir);
+    v9fs_readdir_init(&f->fs_reclaim.dir);
+
     return f;
 }
 
@@ -1640,6 +1643,9 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
 
     while (1) {
         v9fs_path_init(&path);
+
+        v9fs_readdir_lock(&fidp->fs.dir);
+
         err = v9fs_co_readdir_r(pdu, fidp, dent, &result);
         if (err || !result) {
             break;
@@ -1658,6 +1664,9 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         }
         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "S", &v9stat);
+
+        v9fs_readdir_unlock(&fidp->fs.dir);
+
         if ((len != (v9stat.size + 2)) || ((count + len) > max_count)) {
             /* Ran out of buffer. Set dir back to old position and return */
             v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
@@ -1672,6 +1681,8 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         saved_dir_pos = dent->d_off;
     }
 
+    v9fs_readdir_unlock(&fidp->fs.dir);
+
     g_free(dent);
     v9fs_path_free(&path);
     if (err < 0) {
@@ -1819,6 +1830,8 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
     dent = g_malloc(sizeof(struct dirent));
 
     while (1) {
+        v9fs_readdir_lock(&fidp->fs.dir);
+
         err = v9fs_co_readdir_r(pdu, fidp, dent, &result);
         if (err || !result) {
             break;
@@ -1826,6 +1839,8 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
         v9fs_string_init(&name);
         v9fs_string_sprintf(&name, "%s", dent->d_name);
         if ((count + v9fs_readdir_data_size(&name)) > max_count) {
+            v9fs_readdir_unlock(&fidp->fs.dir);
+
             /* Ran out of buffer. Set dir back to old position and return */
             v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
             v9fs_string_free(&name);
@@ -1847,6 +1862,9 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
         len = pdu_marshal(pdu, 11 + count, "Qqbs",
                           &qid, dent->d_off,
                           dent->d_type, &name);
+
+        v9fs_readdir_unlock(&fidp->fs.dir);
+
         if (len < 0) {
             v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
             v9fs_string_free(&name);
@@ -1857,6 +1875,9 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
         v9fs_string_free(&name);
         saved_dir_pos = dent->d_off;
     }
+
+    v9fs_readdir_unlock(&fidp->fs.dir);
+
     g_free(dent);
     if (err < 0) {
         return err;
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 92ee309ef4ba..46d787627a4c 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -169,8 +169,24 @@ typedef struct V9fsXattr
 
 typedef struct V9fsDir {
     DIR *stream;
+    QemuMutex readdir_mutex;
 } V9fsDir;
 
+static inline void v9fs_readdir_lock(V9fsDir *dir)
+{
+    qemu_mutex_lock(&dir->readdir_mutex);
+}
+
+static inline void v9fs_readdir_unlock(V9fsDir *dir)
+{
+    qemu_mutex_unlock(&dir->readdir_mutex);
+}
+
+static inline void v9fs_readdir_init(V9fsDir *dir)
+{
+    qemu_mutex_init(&dir->readdir_mutex);
+}
+
 /*
  * Filled by fs driver on open and other
  * calls.

[Qemu-devel] [PATCH 4/4] 9p: switch back to readdir()

[Greg Kurz]

This patch changes the 9p code to use readdir() again instead of
readdir_r(), which is deprecated in glibc 2.24.

All the locking was put in place by a previous patch.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 fsdev/file-op-9p.h  |    3 +--
 hw/9pfs/9p-handle.c |    8 +++-----
 hw/9pfs/9p-local.c  |   20 +++++++++++---------
 hw/9pfs/9p-proxy.c  |    8 +++-----
 hw/9pfs/9p-synth.c  |   23 ++++++++++-------------
 hw/9pfs/9p-synth.h  |    1 +
 hw/9pfs/9p.c        |   21 ++++++---------------
 hw/9pfs/codir.c     |   12 +++++++-----
 hw/9pfs/coth.h      |    3 +--
 9 files changed, 43 insertions(+), 56 deletions(-)

diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 1095fcc95757..55614949740d 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -118,8 +118,7 @@ struct FileOperations
                  int, FsCred *, V9fsFidOpenState *);
     void (*rewinddir)(FsContext *, V9fsFidOpenState *);
     off_t (*telldir)(FsContext *, V9fsFidOpenState *);
-    int (*readdir_r)(FsContext *, V9fsFidOpenState *,
-                     struct dirent *, struct dirent **);
+    struct dirent * (*readdir)(FsContext *, V9fsFidOpenState *);
     void (*seekdir)(FsContext *, V9fsFidOpenState *, off_t);
     ssize_t (*preadv)(FsContext *, V9fsFidOpenState *,
                       const struct iovec *, int, off_t);
diff --git a/hw/9pfs/9p-handle.c b/hw/9pfs/9p-handle.c
index c5df05300700..3d77594f9245 100644
--- a/hw/9pfs/9p-handle.c
+++ b/hw/9pfs/9p-handle.c
@@ -149,11 +149,9 @@ static off_t handle_telldir(FsContext *ctx, V9fsFidOpenState *fs)
     return telldir(fs->dir.stream);
 }
 
-static int handle_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
-                            struct dirent *entry,
-                            struct dirent **result)
+static struct dirent *handle_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return readdir_r(fs->dir.stream, entry, result);
+    return readdir(fs->dir.stream);
 }
 
 static void handle_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
@@ -681,7 +679,7 @@ FileOperations handle_ops = {
     .opendir      = handle_opendir,
     .rewinddir    = handle_rewinddir,
     .telldir      = handle_telldir,
-    .readdir_r    = handle_readdir_r,
+    .readdir      = handle_readdir,
     .seekdir      = handle_seekdir,
     .preadv       = handle_preadv,
     .pwritev      = handle_pwritev,
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 3281acdef164..3f271fcbd2c5 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -388,25 +388,27 @@ static off_t local_telldir(FsContext *ctx, V9fsFidOpenState *fs)
     return telldir(fs->dir.stream);
 }
 
-static int local_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
-                           struct dirent *entry,
-                           struct dirent **result)
+static struct dirent *local_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    int ret;
+    struct dirent *entry;
 
 again:
-    ret = readdir_r(fs->dir.stream, entry, result);
+    entry = readdir(fs->dir.stream);
+    if (!entry) {
+        return NULL;
+    }
+
     if (ctx->export_flags & V9FS_SM_MAPPED) {
         entry->d_type = DT_UNKNOWN;
     } else if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
-        if (!ret && *result != NULL &&
-            !strcmp(entry->d_name, VIRTFS_META_DIR)) {
+        if (!strcmp(entry->d_name, VIRTFS_META_DIR)) {
             /* skp the meta data directory */
             goto again;
         }
         entry->d_type = DT_UNKNOWN;
     }
-    return ret;
+
+    return entry;
 }
 
 static void local_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
@@ -1254,7 +1256,7 @@ FileOperations local_ops = {
     .opendir = local_opendir,
     .rewinddir = local_rewinddir,
     .telldir = local_telldir,
-    .readdir_r = local_readdir_r,
+    .readdir = local_readdir,
     .seekdir = local_seekdir,
     .preadv = local_preadv,
     .pwritev = local_pwritev,
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 63b074781589..f265501eac1d 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -678,11 +678,9 @@ static off_t proxy_telldir(FsContext *ctx, V9fsFidOpenState *fs)
     return telldir(fs->dir.stream);
 }
 
-static int proxy_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
-                           struct dirent *entry,
-                           struct dirent **result)
+static struct dirent *proxy_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return readdir_r(fs->dir.stream, entry, result);
+    return readdir(fs->dir.stream);
 }
 
 static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
@@ -1192,7 +1190,7 @@ FileOperations proxy_ops = {
     .opendir      = proxy_opendir,
     .rewinddir    = proxy_rewinddir,
     .telldir      = proxy_telldir,
-    .readdir_r    = proxy_readdir_r,
+    .readdir      = proxy_readdir,
     .seekdir      = proxy_seekdir,
     .preadv       = proxy_preadv,
     .pwritev      = proxy_pwritev,
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 8e504130d94b..73c8be816bbb 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -223,8 +223,8 @@ static void v9fs_synth_direntry(V9fsSynthNode *node,
     entry->d_off = off + 1;
 }
 
-static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
-                                 struct dirent **result, off_t off)
+static struct dirent *v9fs_synth_get_dentry(V9fsSynthNode *dir,
+                                            struct dirent *entry, off_t off)
 {
     int i = 0;
     V9fsSynthNode *node;
@@ -240,25 +240,22 @@ static int v9fs_synth_get_dentry(V9fsSynthNode *dir, struct dirent *entry,
     rcu_read_unlock();
     if (!node) {
         /* end of directory */
-        *result = NULL;
-        return 0;
+        return NULL;
     }
     v9fs_synth_direntry(node, entry, off);
-    *result = entry;
-    return 0;
+    return entry;
 }
 
-static int v9fs_synth_readdir_r(FsContext *ctx, V9fsFidOpenState *fs,
-                                struct dirent *entry, struct dirent **result)
+static struct dirent *v9fs_synth_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    int ret;
+    struct dirent *entry;
     V9fsSynthOpenState *synth_open = fs->private;
     V9fsSynthNode *node = synth_open->node;
-    ret = v9fs_synth_get_dentry(node, entry, result, synth_open->offset);
-    if (!ret && *result != NULL) {
+    entry = v9fs_synth_get_dentry(node, &synth_open->dent, synth_open->offset);
+    if (entry) {
         synth_open->offset++;
     }
-    return ret;
+    return entry;
 }
 
 static int v9fs_synth_open(FsContext *ctx, V9fsPath *fs_path,
@@ -544,7 +541,7 @@ FileOperations synth_ops = {
     .opendir      = v9fs_synth_opendir,
     .rewinddir    = v9fs_synth_rewinddir,
     .telldir      = v9fs_synth_telldir,
-    .readdir_r    = v9fs_synth_readdir_r,
+    .readdir      = v9fs_synth_readdir,
     .seekdir      = v9fs_synth_seekdir,
     .preadv       = v9fs_synth_preadv,
     .pwritev      = v9fs_synth_pwritev,
diff --git a/hw/9pfs/9p-synth.h b/hw/9pfs/9p-synth.h
index 82962512a199..7c8441bd6cf9 100644
--- a/hw/9pfs/9p-synth.h
+++ b/hw/9pfs/9p-synth.h
@@ -40,6 +40,7 @@ struct V9fsSynthNode {
 typedef struct V9fsSynthOpenState {
     off_t offset;
     V9fsSynthNode *node;
+    struct dirent dent;
 } V9fsSynthOpenState;
 
 extern int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode,
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 3ed077b25b98..ef7ef3f64ca7 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1631,7 +1631,7 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
     int32_t count = 0;
     struct stat stbuf;
     off_t saved_dir_pos;
-    struct dirent *dent, *result;
+    struct dirent *dent;
 
     /* save the directory position */
     saved_dir_pos = v9fs_co_telldir(pdu, fidp);
@@ -1639,15 +1639,13 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         return saved_dir_pos;
     }
 
-    dent = g_malloc(sizeof(struct dirent));
-
     while (1) {
         v9fs_path_init(&path);
 
         v9fs_readdir_lock(&fidp->fs.dir);
 
-        err = v9fs_co_readdir_r(pdu, fidp, dent, &result);
-        if (err || !result) {
+        err = v9fs_co_readdir(pdu, fidp, &dent);
+        if (err || !dent) {
             break;
         }
         err = v9fs_co_name_to_path(pdu, &fidp->path, dent->d_name, &path);
@@ -1672,7 +1670,6 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
             v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
             v9fs_stat_free(&v9stat);
             v9fs_path_free(&path);
-            g_free(dent);
             return count;
         }
         count += len;
@@ -1683,7 +1680,6 @@ static int v9fs_do_readdir_with_stat(V9fsPDU *pdu,
 
     v9fs_readdir_unlock(&fidp->fs.dir);
 
-    g_free(dent);
     v9fs_path_free(&path);
     if (err < 0) {
         return err;
@@ -1819,7 +1815,7 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
     int len, err = 0;
     int32_t count = 0;
     off_t saved_dir_pos;
-    struct dirent *dent, *result;
+    struct dirent *dent;
 
     /* save the directory position */
     saved_dir_pos = v9fs_co_telldir(pdu, fidp);
@@ -1827,13 +1823,11 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
         return saved_dir_pos;
     }
 
-    dent = g_malloc(sizeof(struct dirent));
-
     while (1) {
         v9fs_readdir_lock(&fidp->fs.dir);
 
-        err = v9fs_co_readdir_r(pdu, fidp, dent, &result);
-        if (err || !result) {
+        err = v9fs_co_readdir(pdu, fidp, &dent);
+        if (err || !dent) {
             break;
         }
         v9fs_string_init(&name);
@@ -1844,7 +1838,6 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
             /* Ran out of buffer. Set dir back to old position and return */
             v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
             v9fs_string_free(&name);
-            g_free(dent);
             return count;
         }
         /*
@@ -1868,7 +1861,6 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
         if (len < 0) {
             v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
             v9fs_string_free(&name);
-            g_free(dent);
             return len;
         }
         count += len;
@@ -1878,7 +1870,6 @@ static int v9fs_do_readdir(V9fsPDU *pdu,
 
     v9fs_readdir_unlock(&fidp->fs.dir);
 
-    g_free(dent);
     if (err < 0) {
         return err;
     }
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 8e3ba17baeed..80373dce5261 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -17,8 +17,7 @@
 #include "qemu/coroutine.h"
 #include "coth.h"
 
-int v9fs_co_readdir_r(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent *dent,
-                      struct dirent **result)
+int v9fs_co_readdir(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent **dent)
 {
     int err;
     V9fsState *s = pdu->s;
@@ -28,11 +27,14 @@ int v9fs_co_readdir_r(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent *dent,
     }
     v9fs_co_run_in_worker(
         {
-            errno = 0;
-            err = s->ops->readdir_r(&s->ctx, &fidp->fs, dent, result);
-            if (!*result && errno) {
+            struct dirent *entry;
+            int old_errno = errno;
+
+            entry = s->ops->readdir(&s->ctx, &fidp->fs);
+            if (!entry && errno != old_errno) {
                 err = -errno;
             } else {
+                *dent = entry;
                 err = 0;
             }
         });
diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
index 6adc773d4664..5b02a63ad9fd 100644
--- a/hw/9pfs/coth.h
+++ b/hw/9pfs/coth.h
@@ -49,8 +49,7 @@
 
 extern void co_run_in_worker_bh(void *);
 extern int v9fs_co_readlink(V9fsPDU *, V9fsPath *, V9fsString *);
-extern int v9fs_co_readdir_r(V9fsPDU *, V9fsFidState *,
-                           struct dirent *, struct dirent **result);
+extern int v9fs_co_readdir(V9fsPDU *, V9fsFidState *, struct dirent **);
 extern off_t v9fs_co_telldir(V9fsPDU *, V9fsFidState *);
 extern void v9fs_co_seekdir(V9fsPDU *, V9fsFidState *, off_t);
 extern void v9fs_co_rewinddir(V9fsPDU *, V9fsFidState *);

Re: [Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Peter Maydell]

On 2 June 2016 at 09:51, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> The readdir_r() function has a broken design and should not be used anymore.
> It is expected to be obsoleted in a future version of POSIX.1:
>
> http://austingroupbugs.net/view.php?id=696#c2857
>
> Glibc has already announced that 2.24 (scheduled for August 2016) will
> deprecates readdir_r() and encourages people to use readdir() with
> external synchronization instead.

> Since POSIX.1 will require readdir() to be thread-safe when employed on
> different directory streams, and glibc already does that, the choice
> was made to have per-directory locking.

AIUI the argument is that all sensible implementations of readdir()
already provide the thread-safety guarantees POSIX is going to
specify, but have you tested this on one of the BSDs or OSX?
(and/or checked their current readdir implementation...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Greg Kurz]

On Thu, 2 Jun 2016 10:33:06 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 2 June 2016 at 09:51, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > The readdir_r() function has a broken design and should not be used anymore.
> > It is expected to be obsoleted in a future version of POSIX.1:
> >
> > http://austingroupbugs.net/view.php?id=696#c2857
> >
> > Glibc has already announced that 2.24 (scheduled for August 2016) will
> > deprecates readdir_r() and encourages people to use readdir() with
> > external synchronization instead.  
> 
> > Since POSIX.1 will require readdir() to be thread-safe when employed on
> > different directory streams, and glibc already does that, the choice
> > was made to have per-directory locking.  
> 
> AIUI the argument is that all sensible implementations of readdir()
> already provide the thread-safety guarantees POSIX is going to
> specify, but have you tested this on one of the BSDs or OSX?
> (and/or checked their current readdir implementation...)
> 

No I haven't because "VirtFS is supported only on Linux" at the moment.

But thanks for raising the flag: it reminds me that there's ongoing
work to support VirtFS on win32 hosts and I should also Cc Michael
Fritscher.

Thanks !

--
Greg

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Peter Maydell]

On 2 June 2016 at 10:42, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Thu, 2 Jun 2016 10:33:06 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> AIUI the argument is that all sensible implementations of readdir()
>> already provide the thread-safety guarantees POSIX is going to
>> specify, but have you tested this on one of the BSDs or OSX?
>> (and/or checked their current readdir implementation...)

> No I haven't because "VirtFS is supported only on Linux" at the moment.

Ah, so it is, I hadn't noticed we disabled it in configure for
non-Linux hosts.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Michael Fritscher]

[-- Attachment #1.1: Type: text/plain, Size: 4655 bytes --]

Am 02.06.2016 um 11:42 schrieb Greg Kurz:
> On Thu, 2 Jun 2016 10:33:06 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 2 June 2016 at 09:51, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>>> The readdir_r() function has a broken design and should not be used anymore.
>>> It is expected to be obsoleted in a future version of POSIX.1:
>>>
>>> http://austingroupbugs.net/view.php?id=696#c2857
>>>
>>> Glibc has already announced that 2.24 (scheduled for August 2016) will
>>> deprecates readdir_r() and encourages people to use readdir() with
>>> external synchronization instead.
>>
>>> Since POSIX.1 will require readdir() to be thread-safe when employed on
>>> different directory streams, and glibc already does that, the choice
>>> was made to have per-directory locking.
>>
>> AIUI the argument is that all sensible implementations of readdir()
>> already provide the thread-safety guarantees POSIX is going to
>> specify, but have you tested this on one of the BSDs or OSX?
>> (and/or checked their current readdir implementation...)
>>
>
> No I haven't because "VirtFS is supported only on Linux" at the moment.
>
> But thanks for raising the flag: it reminds me that there's ongoing
> work to support VirtFS on win32 hosts and I should also Cc Michael
> Fritscher.

Hi,
thanks for CCing me!

I've digged a bit about the readdir<89 problem.
Following question: So we should relay on the thread safeness on 
readdir() in the case of different directory streams?

I looked into the MingW implementation of it (see below). If I see it 
correctly it seems to be threadsafe on the case of different directory 
streams as well. But I didn't found any info about whether findfirst is 
- but no warning regarding it on the MSDN 
(https://msdn.microsoft.com/en-us/library/6tkkkc1y.aspx)

It is clearly NOT threadsafe using it on the same dir stream.

I think we need to lock based on the pointer saved in V9fsFidOpenState 
fs->dir . Do we have a way to archive something like this:

lock(fs->dir); //if there is already a lock named the address stored in 
fs->dir then wait
readdir(fs_dir,...);
unlock(fs->dir);

? Else we would have to track all the fs->dir, which is a bit of work 
I'm afraid (some kind of hashset or similiar would be usefull then)...

Best regards,
Michael Fritscher

/*
  * readdir
  *
  * Return a pointer to a dirent structure filled with the information 
on the
  * next entry in the directory.
  */
struct _tdirent *
_treaddir (_TDIR * dirp)
{
   errno = 0;

   /* Check for valid DIR struct. */
   if (!dirp)
     {
       errno = EFAULT;
       return (struct _tdirent *) 0;
     }

   if (dirp->dd_stat < 0)
     {
       /* We have already returned all files in the directory
        * (or the structure has an invalid dd_stat). */
       return (struct _tdirent *) 0;
     }
   else if (dirp->dd_stat == 0)
     {
       /* We haven't started the search yet. */
       /* Start the search */
       dirp->dd_handle = _tfindfirst (dirp->dd_name, &(dirp->dd_dta));

       if (dirp->dd_handle == -1)
	{
	  /* Whoops! Seems there are no files in that
	   * directory. */
	  dirp->dd_stat = -1;
	}
       else
	{
	  dirp->dd_stat = 1;
	}
     }
   else
     {
       /* Get the next search entry. */
       if (_tfindnext (dirp->dd_handle, &(dirp->dd_dta)))
	{
	  /* We are off the end or otherwise error.
	     _findnext sets errno to ENOENT if no more file
	     Undo this. */
	  DWORD winerr = GetLastError ();
	  if (winerr == ERROR_NO_MORE_FILES)
	    errno = 0;
	  _findclose (dirp->dd_handle);
	  dirp->dd_handle = -1;
	  dirp->dd_stat = -1;
	}
       else
	{
	  /* Update the status to indicate the correct
	   * number. */
	  dirp->dd_stat++;
	}
     }

   if (dirp->dd_stat > 0)
     {
       /* Successfully got an entry. Everything about the file is
        * already appropriately filled in except the length of the
        * file name. */
       dirp->dd_dir.d_namlen = _tcslen (dirp->dd_dta.name);
       _tcscpy (dirp->dd_dir.d_name, dirp->dd_dta.name);
       return &dirp->dd_dir;
     }

   return (struct _tdirent *) 0;
}

-- 
ZfT - Zentrum für Telematik e.V.
Michael Fritscher
Magdalene-Schoch-Straße 5
97074 Würzburg
Tel: +49 (931) 615 633 - 57
Fax: +49 (931) 615 633 - 11
Email: michael.fritscher@telematik-zentrum.de
Web: http://www.telematik-zentrum.de

Vorstand:
Prof. Dr. Klaus Schilling, Hans-Joachim Leistner
Sitz: Gerbrunn
USt.-ID Nr.: DE 257 244 580, Steuer-Nr.:  257/111/70203
Amtsgericht Würzburg, Vereinsregister-Nr.: VR 200 167

[-- Attachment #1.2: michael_fritscher.vcf --]
[-- Type: text/x-vcard, Size: 352 bytes --]

begin:vcard
fn:Michael Fritscher
n:Fritscher;Michael
org;quoted-printable:Zentrum f=C3=BCr Telematik
adr:;;Allesgrundweg 12;Gerbrunn;Bayern;97218;Deutschland
email;internet:michael.fritscher@telematik-zentrum.de
tel;work:+49 (931) 3 29 29 54 - 21
tel;fax:+49 (931) 3 29 29 54 - 11
url:http://www.telematik-zentrum.de
version:2.1
end:vcard


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4423 bytes --]

Re: [Qemu-devel] [PATCH 0/4] 9p: get rid of readdir_r()

[Greg Kurz]

On Thu, 2 Jun 2016 15:59:29 +0200
Michael Fritscher <michael.fritscher@telematik-zentrum.de> wrote:

> Am 02.06.2016 um 11:42 schrieb Greg Kurz:
> > On Thu, 2 Jun 2016 10:33:06 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >  
> >> On 2 June 2016 at 09:51, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:  
> >>> The readdir_r() function has a broken design and should not be used anymore.
> >>> It is expected to be obsoleted in a future version of POSIX.1:
> >>>
> >>> http://austingroupbugs.net/view.php?id=696#c2857
> >>>
> >>> Glibc has already announced that 2.24 (scheduled for August 2016) will
> >>> deprecates readdir_r() and encourages people to use readdir() with
> >>> external synchronization instead.  
> >>  
> >>> Since POSIX.1 will require readdir() to be thread-safe when employed on
> >>> different directory streams, and glibc already does that, the choice
> >>> was made to have per-directory locking.  
> >>
> >> AIUI the argument is that all sensible implementations of readdir()
> >> already provide the thread-safety guarantees POSIX is going to
> >> specify, but have you tested this on one of the BSDs or OSX?
> >> (and/or checked their current readdir implementation...)
> >>  
> >
> > No I haven't because "VirtFS is supported only on Linux" at the moment.
> >
> > But thanks for raising the flag: it reminds me that there's ongoing
> > work to support VirtFS on win32 hosts and I should also Cc Michael
> > Fritscher.  
> 
> Hi,
> thanks for CCing me!
> 

Hi Michael !

> I've digged a bit about the readdir<89 problem.
> Following question: So we should relay on the thread safeness on 
> readdir() in the case of different directory streams?
> 
> I looked into the MingW implementation of it (see below). If I see it 
> correctly it seems to be threadsafe on the case of different directory 
> streams as well. 

Cool.

> But I didn't found any info about whether findfirst is 
> - but no warning regarding it on the MSDN 
> (https://msdn.microsoft.com/en-us/library/6tkkkc1y.aspx)
> 

Heh the windows API is a bit similar to readdir_r(): the caller
has a handle to pass to all find* calls. So it is likely to
be reentrant, but we would need to see the source to be sure ;)

> It is clearly NOT threadsafe using it on the same dir stream.
> 
> I think we need to lock based on the pointer saved in V9fsFidOpenState 
> fs->dir . Do we have a way to archive something like this:
> 
> lock(fs->dir); //if there is already a lock named the address stored in 
> fs->dir then wait
> readdir(fs_dir,...);
> unlock(fs->dir);
> 
> ? Else we would have to track all the fs->dir, which is a bit of work 
> I'm afraid (some kind of hashset or similiar would be usefull then)...
> 

This series gets rid of readdir_r() completely and already addresses all
the locking around critical sections. So you don't have to implement your
own version of readdir_r() anymore.

Maybe you can rebase your patchset against:

https://github.com/gkurz/qemu.git 9p-next

Cheers.

--
Greg

> Best regards,
> Michael Fritscher
> 
> /*
>   * readdir
>   *
>   * Return a pointer to a dirent structure filled with the information 
> on the
>   * next entry in the directory.
>   */
> struct _tdirent *
> _treaddir (_TDIR * dirp)
> {
>    errno = 0;
> 
>    /* Check for valid DIR struct. */
>    if (!dirp)
>      {
>        errno = EFAULT;
>        return (struct _tdirent *) 0;
>      }
> 
>    if (dirp->dd_stat < 0)
>      {
>        /* We have already returned all files in the directory
>         * (or the structure has an invalid dd_stat). */
>        return (struct _tdirent *) 0;
>      }
>    else if (dirp->dd_stat == 0)
>      {
>        /* We haven't started the search yet. */
>        /* Start the search */
>        dirp->dd_handle = _tfindfirst (dirp->dd_name, &(dirp->dd_dta));
> 
>        if (dirp->dd_handle == -1)
> 	{
> 	  /* Whoops! Seems there are no files in that
> 	   * directory. */
> 	  dirp->dd_stat = -1;
> 	}
>        else
> 	{
> 	  dirp->dd_stat = 1;
> 	}
>      }
>    else
>      {
>        /* Get the next search entry. */
>        if (_tfindnext (dirp->dd_handle, &(dirp->dd_dta)))
> 	{
> 	  /* We are off the end or otherwise error.
> 	     _findnext sets errno to ENOENT if no more file
> 	     Undo this. */
> 	  DWORD winerr = GetLastError ();
> 	  if (winerr == ERROR_NO_MORE_FILES)
> 	    errno = 0;
> 	  _findclose (dirp->dd_handle);
> 	  dirp->dd_handle = -1;
> 	  dirp->dd_stat = -1;
> 	}
>        else
> 	{
> 	  /* Update the status to indicate the correct
> 	   * number. */
> 	  dirp->dd_stat++;
> 	}
>      }
> 
>    if (dirp->dd_stat > 0)
>      {
>        /* Successfully got an entry. Everything about the file is
>         * already appropriately filled in except the length of the
>         * file name. */
>        dirp->dd_dir.d_namlen = _tcslen (dirp->dd_dta.name);
>        _tcscpy (dirp->dd_dir.d_name, dirp->dd_dta.name);
>        return &dirp->dd_dir;
>      }
> 
>    return (struct _tdirent *) 0;
> }
> 

Re: [Qemu-devel] [PATCH 1/4] 9p: drop useless out: label

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

On 06/02/2016 02:52 AM, Greg Kurz wrote:
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/9pfs/9p.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/4] 9p: introduce the V9fsDir type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On 06/02/2016 02:52 AM, Greg Kurz wrote:
> If we are to switch back to readdir(), we need a more complex type than
> DIR * to be able to serialize concurrent accesses to the directory stream.
> 
> This patch introduces a placeholder type and fixes all users.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/9pfs/9p-handle.c |   18 +++++++++---------
>  hw/9pfs/9p-local.c  |   18 +++++++++---------
>  hw/9pfs/9p-proxy.c  |   20 ++++++++++----------
>  hw/9pfs/9p.c        |   12 ++++++------
>  hw/9pfs/9p.h        |    6 +++++-
>  5 files changed, 39 insertions(+), 35 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/4] 9p: add locking to V9fsDir

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On 06/02/2016 02:52 AM, Greg Kurz wrote:
> If several threads call concurrently readdir() with the same directory

s/call concurrently/concurrently call/

> stream pointer, it is possible that they all get a pointer to the same
> dirent structure, whose content is overwritten each time readdir() is
> called.
> 
> We must thus serialize accesses to the dirent structure.
> 
> This may be achieved with a mutex like below:
> 
> lock_mutex();
> 
> readdir();
> 
> // work with the dirent
> 
> unlock_mutex();
> 
> This patch adds all the locking, to prepare the switch to readdir().
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/9pfs/9p.c |   21 +++++++++++++++++++++
>  hw/9pfs/9p.h |   16 ++++++++++++++++
>  2 files changed, 37 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/4] 9p: switch back to readdir()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On 06/02/2016 02:52 AM, Greg Kurz wrote:
> This patch changes the 9p code to use readdir() again instead of
> readdir_r(), which is deprecated in glibc 2.24.
> 
> All the locking was put in place by a previous patch.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---


> +++ b/hw/9pfs/codir.c
> @@ -17,8 +17,7 @@
>  #include "qemu/coroutine.h"
>  #include "coth.h"
>  
> -int v9fs_co_readdir_r(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent *dent,
> -                      struct dirent **result)
> +int v9fs_co_readdir(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent **dent)
>  {
>      int err;
>      V9fsState *s = pdu->s;
> @@ -28,11 +27,14 @@ int v9fs_co_readdir_r(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent *dent,
>      }
>      v9fs_co_run_in_worker(
>          {
> -            errno = 0;
> -            err = s->ops->readdir_r(&s->ctx, &fidp->fs, dent, result);
> -            if (!*result && errno) {
> +            struct dirent *entry;
> +            int old_errno = errno;
> +
> +            entry = s->ops->readdir(&s->ctx, &fidp->fs);
> +            if (!entry && errno != old_errno) {
>                  err = -errno;

Not safe. The only safe way to check errno after readdir() is to assign
it to 0 before readdir().

>              } else {
> +                *dent = entry;
>                  err = 0;
>              }
>          });


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/4] 9p: switch back to readdir()

[Greg Kurz]

On Thu, 2 Jun 2016 15:00:26 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 06/02/2016 02:52 AM, Greg Kurz wrote:
> > This patch changes the 9p code to use readdir() again instead of
> > readdir_r(), which is deprecated in glibc 2.24.
> > 
> > All the locking was put in place by a previous patch.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---  
> 
> 
> > +++ b/hw/9pfs/codir.c
> > @@ -17,8 +17,7 @@
> >  #include "qemu/coroutine.h"
> >  #include "coth.h"
> >  
> > -int v9fs_co_readdir_r(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent *dent,
> > -                      struct dirent **result)
> > +int v9fs_co_readdir(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent **dent)
> >  {
> >      int err;
> >      V9fsState *s = pdu->s;
> > @@ -28,11 +27,14 @@ int v9fs_co_readdir_r(V9fsPDU *pdu, V9fsFidState *fidp, struct dirent *dent,
> >      }
> >      v9fs_co_run_in_worker(
> >          {
> > -            errno = 0;
> > -            err = s->ops->readdir_r(&s->ctx, &fidp->fs, dent, result);
> > -            if (!*result && errno) {
> > +            struct dirent *entry;
> > +            int old_errno = errno;
> > +
> > +            entry = s->ops->readdir(&s->ctx, &fidp->fs);
> > +            if (!entry && errno != old_errno) {
> >                  err = -errno;  
> 
> Not safe. The only safe way to check errno after readdir() is to assign
> it to 0 before readdir().
> 

Yeah, you're right, errno could already be set to one of the values
returned by readdir() in case of error... 

> >              } else {
> > +                *dent = entry;
> >                  err = 0;
> >              }
> >          });  
> 
>