| Message ID | 20220206200719.74464-5-wwcohen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9p: Add support for darwin | expand |
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“? > > + /* 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! > 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. 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. > > } > > 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? > 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) > >
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) >> >>
On Sonntag, 6. Februar 2022 21:07:12 CET Will Cohen wrote: > From: Keno Fischer <keno@juliacomputing.com> > > On darwin d_seekoff exists, but is optional and does not seem to > be commonly used by file systems. Use `telldir` instead to obtain > the seek offset and inject it into d_seekoff, and create a > qemu_dirent_off helper to call it appropriately when appropriate. > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > [Michael Roitzsch: - Rebase for NixOS] > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com> > [Will Cohen: - Adjust to pass testing > - Ensure that d_seekoff is filled using telldir > on darwin, and create qemu_dirent_off helper > to decide which to access] > [Fabian Franz: - Add telldir error handling for darwin] > [Will Cohen: - Ensure that telldir error handling uses > signed int] > Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com> > Signed-off-by: Will Cohen <wwcohen@gmail.com> > --- > hw/9pfs/9p-local.c | 9 +++++++++ > hw/9pfs/9p-proxy.c | 16 +++++++++++++++- > hw/9pfs/9p-synth.c | 4 ++++ > hw/9pfs/9p-util.h | 17 +++++++++++++++++ > hw/9pfs/9p.c | 15 +++++++++++++-- > hw/9pfs/codir.c | 7 +++++++ > 6 files changed, 65 insertions(+), 3 deletions(-) > > 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); > + /* If telldir fails, fail the entire readdir call */ > + if (td < 0) { > + return NULL; > + } > + entry->d_seekoff = td; > +#endif > if (!entry) { > return NULL; > } 'entry' may be NULL, so the 'if (!entry) {' check should be before the Darwin specific code to avoid a crash on macOS. > 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. > + */ > + Nitpicking: no blank line here please. > +inline off_t qemu_dirent_off(struct dirent *dent) > +{ > +#ifdef CONFIG_DARWIN > + return dent->d_seekoff; > +#else > + return dent->d_off; > +#endif > +} > 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; > + } > } That check is no longer needed here, is it? > > 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; > + } Likewise: is this check still needed? > 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 Why can't the header not be included here? Obvious preference would be to use qemu_dirent_off() here as well, to have control at one central code location. > } > > /* restore (last) saved position */
On Mon, Feb 7, 2022 at 9:41 AM Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Sonntag, 6. Februar 2022 21:07:12 CET Will Cohen wrote: > > From: Keno Fischer <keno@juliacomputing.com> > > > > On darwin d_seekoff exists, but is optional and does not seem to > > be commonly used by file systems. Use `telldir` instead to obtain > > the seek offset and inject it into d_seekoff, and create a > > qemu_dirent_off helper to call it appropriately when appropriate. > > > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > > [Michael Roitzsch: - Rebase for NixOS] > > Signed-off-by: Michael Roitzsch <reactorcontrol@icloud.com> > > [Will Cohen: - Adjust to pass testing > > - Ensure that d_seekoff is filled using telldir > > on darwin, and create qemu_dirent_off helper > > to decide which to access] > > [Fabian Franz: - Add telldir error handling for darwin] > > [Will Cohen: - Ensure that telldir error handling uses > > signed int] > > Signed-off-by: Fabian Franz <fabianfranz.oss@gmail.com> > > Signed-off-by: Will Cohen <wwcohen@gmail.com> > > --- > > hw/9pfs/9p-local.c | 9 +++++++++ > > hw/9pfs/9p-proxy.c | 16 +++++++++++++++- > > hw/9pfs/9p-synth.c | 4 ++++ > > hw/9pfs/9p-util.h | 17 +++++++++++++++++ > > hw/9pfs/9p.c | 15 +++++++++++++-- > > hw/9pfs/codir.c | 7 +++++++ > > 6 files changed, 65 insertions(+), 3 deletions(-) > > > > 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); > > + /* If telldir fails, fail the entire readdir call */ > > + if (td < 0) { > > + return NULL; > > + } > > + entry->d_seekoff = td; > > +#endif > > if (!entry) { > > return NULL; > > } > > 'entry' may be NULL, so the 'if (!entry) {' check should be before the > Darwin > specific code to avoid a crash on macOS. > Adjusting for v5. > > > 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. > > + */ > > + > > Nitpicking: no blank line here please. > Adjusting for v5. > > > +inline off_t qemu_dirent_off(struct dirent *dent) > > +{ > > +#ifdef CONFIG_DARWIN > > + return dent->d_seekoff; > > +#else > > + return dent->d_off; > > +#endif > > +} > > 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; > > + } > > } > > That check is no longer needed here, is it? > Correct! Adjusting 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; > > + } > > Likewise: is this check still needed? > As above, removing for v5. > > > 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 > > Why can't the header not be included here? Obvious preference would be to > use > qemu_dirent_off() here as well, to have control at one central code > location. > Only my own imprecision in organizing the includes correctly. After including "9p-xattr.h" as well, this works. Will adjust for v5. > > > } > > > > /* restore (last) saved position */ > > >
On Montag, 7. Februar 2022 14:52:58 CET Will Cohen wrote: > 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 See below ... > >> > >> } > >> > >> 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. As requested by me in this v4, this will be 3 times in v5. So I assume that qualifies the dedicated helper function. :) As an alternative you could make the helper function a macro instead, then you could use it in 9p-synth.c as well, which would make it 4 times. But I don't mind about that one. Best regards, Christian Schoenebeck > > 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)
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); + /* If telldir fails, fail the entire readdir call */ + if (td < 0) { + return NULL; + } + entry->d_seekoff = td; +#endif if (!entry) { return NULL; } 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 +} 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; + } } 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; + } 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 */