qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 04/11] 9p: darwin: Handle struct dirent differences


From: Will Cohen
Subject: Re: [PATCH v4 04/11] 9p: darwin: Handle struct dirent differences
Date: Mon, 7 Feb 2022 08:52:58 -0500



On Mon, Feb 7, 2022 at 4:53 AM Fabian Franz <fabianfranz.oss@gmail.com> wrote:
Comments inline:

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 1a5e3eed73..7137a28109 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -559,6 +559,15 @@ static struct dirent *local_readdir(FsContext *ctx, V9fsFidOpenState *fs)

 again:
     entry = readdir(fs->dir.stream);
+#ifdef CONFIG_DARWIN
+    int td;
+    td = telldir(fs->dir.stream);

Maybe call this „off“?

Yes, off is better. Will adjust for v5.
 

+    /* If telldir fails, fail the entire readdir call */
+    if (td < 0) {
+        return NULL;
+    }
+    entry->d_seekoff = td;
+#endif
     if (!entry) {
         return NULL;
     }

This needs to be before the #ifdef!

Good catch, will adjust for v5. I moved it around twice and forgot to put it in the right place.
 


diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index b1664080d8..8b4b5cf7dc 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -706,7 +706,21 @@ static off_t proxy_telldir(FsContext *ctx, V9fsFidOpenState *fs)

 static struct dirent *proxy_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return readdir(fs->dir.stream);
+    struct dirent *entry;
+    entry = readdir(fs->dir.stream);
+#ifdef CONFIG_DARWIN
+    if (!entry) {
+        return NULL;
+    }
+    int td;
+    td = telldir(fs->dir.stream);
+    /* If telldir fails, fail the entire readdir call */
+    if (td < 0) {
+        return NULL;
+    }
+    entry->d_seekoff = td;
+#endif
+    return entry;
 }

 static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 4a4a776d06..e264a03eef 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -222,7 +222,11 @@ static void synth_direntry(V9fsSynthNode *node,
 {
     strcpy(entry->d_name, node->name);
     entry->d_ino = node->attr->inode;
+#ifdef CONFIG_DARWIN
+    entry->d_seekoff = off + 1;
+#else
     entry->d_off = off + 1;
+#endif
 }

 static struct dirent *synth_get_dentry(V9fsSynthNode *dir,
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 546f46dc7d..accbec9987 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -79,3 +79,20 @@ ssize_t fremovexattrat_nofollow(int dirfd, const char *filename,
                                 const char *name);

 #endif
+
+
+/**
+ * Darwin has d_seekoff, which appears to function similarly to d_off.
+ * However, it does not appear to be supported on all file systems,
+ * so ensure it is manually injected earlier and call here when
+ * needed.
+ */
+
+inline off_t qemu_dirent_off(struct dirent *dent)
+{
+#ifdef CONFIG_DARWIN
+    return dent->d_seekoff;
+#else
+    return dent->d_off;
+#endif
+}

Are we sure we want a helper for two times the same ifdef? Deferring to maintainers here however.

Either way works for me too -- my current inclination is to leave it this way (as originally suggested by the maintainers), if for no other reason than that it allows the one comment to be referenced in the case of both uses.
 

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 1563d7b7c6..cf694da354 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -27,6 +27,7 @@
 #include "virtio-9p.h"
 #include "fsdev/qemu-fsdev.h"
 #include "9p-xattr.h"
+#include "9p-util.h"
 #include "coth.h"
 #include "trace.h"
 #include "migration/blocker.h"
@@ -2281,7 +2282,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         count += len;
         v9fs_stat_free(&v9stat);
         v9fs_path_free(&path);
-        saved_dir_pos = dent->d_off;
+        saved_dir_pos = qemu_dirent_off(dent);
+        if (saved_dir_pos < 0) {
+            err = saved_dir_pos;
+            break;
+        }

Do we still need this error-handling? I had removed it in my interdiff patch.

That's correct, it in fact can be removed. d_seekoff yields a __uint64_t (https://developer.apple.com/documentation/kernel/direntry/1415494-d_seekoff?language=objc). Will adjust for v5.
 

     }

     v9fs_readdir_unlock(&fidp->fs.dir);
@@ -2420,6 +2425,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
     V9fsString name;
     int len, err = 0;
     int32_t count = 0;
+    off_t off;
     struct dirent *dent;
     struct stat *st;
     struct V9fsDirEnt *entries = NULL;
@@ -2480,12 +2486,17 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
             qid.version = 0;
         }

+        off = qemu_dirent_off(dent);
+        if (off < 0) {
+            err = off;
+            break;
+        }

Same here - if this can never fail, why add the error handling?

See above.
 


         v9fs_string_init(&name);
         v9fs_string_sprintf(&name, "%s", dent->d_name);

         /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
         len = pdu_marshal(pdu, 11 + count, "Qqbs",
-                          &qid, dent->d_off,
+                          &qid, off,
                           dent->d_type, &name);

         v9fs_string_free(&name);
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 032cce04c4..fac6759a64 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -167,7 +167,14 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
         }

         size += len;
+        /* This conditional statement is identical in
+         * function to qemu_dirent_off, described in 9p-util.h,
+         * since that header cannot be included here. */
+#ifdef CONFIG_DARWIN
+        saved_dir_pos = dent->d_seekoff;
+#else
         saved_dir_pos = dent->d_off;
+#endif
     }

     /* restore (last) saved position */
--
2.32.0 (Apple Git-132)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]