| Message ID | 6205f39b495ee7356478cc53faf5e32289f57182.1527310210.git.keno@alumni.harvard.edu |
|---|---|
| State | New |
| Headers | show |
| Series | 9p: Add support for Darwin | expand |
On Sat, 26 May 2018 01:23:07 -0400 keno@juliacomputing.com wrote: > From: Keno Fischer <keno@alumni.harvard.edu> > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > --- > hw/9pfs/9p-synth.c | 4 ++++ > hw/9pfs/9p.c | 18 ++++++++++++++++-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c > index eb68b42..3c0a6d8 100644 > --- a/hw/9pfs/9p-synth.c > +++ b/hw/9pfs/9p-synth.c > @@ -221,7 +221,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; Hmm... what's that for ? No users in the patchset and the comment below seem to indicate this wouldn't be trusted anyway. > +#else > entry->d_off = off + 1; > +#endif > } > > static struct dirent *synth_get_dentry(V9fsSynthNode *dir, > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 1cfdf7d..49654ae 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1801,7 +1801,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu, > count += len; > v9fs_stat_free(&v9stat); > v9fs_path_free(&path); > +#ifdef CONFIG_DARWIN > + saved_dir_pos = v9fs_co_telldir(pdu, fidp); > +#else > saved_dir_pos = dent->d_off; > +#endif > } > > v9fs_readdir_unlock(&fidp->fs.dir); > @@ -1952,9 +1956,19 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp, > qid.type = 0; > qid.version = 0; > > +#ifdef CONFIG_DARWIN > + // Darwin has d_seekoff, which appears to function > + // analogously to d_off. However, it does not appear > + // to be supported on all file systems, so use > + // telldir for correctness. Please use /* */ > + off_t off = v9fs_co_telldir(pdu, fidp); Please declare local variables at the beginning of the function. Also, v9fs_co_telldir() can fail. This requires proper error handling. > +#else > + off_t off = dent->d_off; > +#endif Please make this a helper and call it in v9fs_do_readdir_with_stat() as well. > + > /* 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_readdir_unlock(&fidp->fs.dir); > @@ -1966,7 +1980,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp, > } > count += len; > v9fs_string_free(&name); > - saved_dir_pos = dent->d_off; > + saved_dir_pos = off; > } > > v9fs_readdir_unlock(&fidp->fs.dir);
>> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c >> index eb68b42..3c0a6d8 100644 >> --- a/hw/9pfs/9p-synth.c >> +++ b/hw/9pfs/9p-synth.c >> @@ -221,7 +221,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; > > Hmm... what's that for ? No users in the patchset and the comment > below seem to indicate this wouldn't be trusted anyway. d_off isn't available on Darwin, so an appropriate ifdef is required here anyway. I figured if the offset is available anyway, I might as well set it, but I can drop this code path if you would prefer. >> + off_t off = v9fs_co_telldir(pdu, fidp); > > Please declare local variables at the beginning of the function. Also, > v9fs_co_telldir() can fail. This requires proper error handling. Will do. >> +#else >> + off_t off = dent->d_off; >> +#endif > > Please make this a helper and call it in v9fs_do_readdir_with_stat() as well. > Will do.
On Thu, 31 May 2018 12:20:28 -0400 Keno Fischer <keno@juliacomputing.com> wrote: > >> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c > >> index eb68b42..3c0a6d8 100644 > >> --- a/hw/9pfs/9p-synth.c > >> +++ b/hw/9pfs/9p-synth.c > >> @@ -221,7 +221,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; > > > > Hmm... what's that for ? No users in the patchset and the comment > > below seem to indicate this wouldn't be trusted anyway. > > d_off isn't available on Darwin, so an appropriate ifdef > is required here anyway. I figured if the offset is available > anyway, I might as well set it, but I can drop > this code path if you would prefer. > Yeah I think I prefer you drop it. > >> + off_t off = v9fs_co_telldir(pdu, fidp); > > > > Please declare local variables at the beginning of the function. Also, > > v9fs_co_telldir() can fail. This requires proper error handling. > > Will do. > > >> +#else > >> + off_t off = dent->d_off; > >> +#endif > > > > Please make this a helper and call it in v9fs_do_readdir_with_stat() as well. > > > > Will do.
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c index eb68b42..3c0a6d8 100644 --- a/hw/9pfs/9p-synth.c +++ b/hw/9pfs/9p-synth.c @@ -221,7 +221,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.c b/hw/9pfs/9p.c index 1cfdf7d..49654ae 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1801,7 +1801,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu, count += len; v9fs_stat_free(&v9stat); v9fs_path_free(&path); +#ifdef CONFIG_DARWIN + saved_dir_pos = v9fs_co_telldir(pdu, fidp); +#else saved_dir_pos = dent->d_off; +#endif } v9fs_readdir_unlock(&fidp->fs.dir); @@ -1952,9 +1956,19 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp, qid.type = 0; qid.version = 0; +#ifdef CONFIG_DARWIN + // Darwin has d_seekoff, which appears to function + // analogously to d_off. However, it does not appear + // to be supported on all file systems, so use + // telldir for correctness. + off_t off = v9fs_co_telldir(pdu, fidp); +#else + off_t off = dent->d_off; +#endif + /* 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_readdir_unlock(&fidp->fs.dir); @@ -1966,7 +1980,7 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp, } count += len; v9fs_string_free(&name); - saved_dir_pos = dent->d_off; + saved_dir_pos = off; } v9fs_readdir_unlock(&fidp->fs.dir);