On Tue, 11 Mar 2025 15:03:13 +0100
Greg Kurz <gr...@kaod.org> wrote:
> On Tue, 11 Mar 2025 12:13:06 +0100
> Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
>
> > On Monday, March 10, 2025 6:10:59 PM CET Greg Kurz wrote:
> > > v9fs_getattr() currently peeks into V9fsFidOpenState to know if a fid
> > > has a valid file descriptor or directory stream. Even though the fields
> > > are accessible, this is an implementation detail of the local backend
> > > that should not be manipulated directly by the server code.
> > >
> > > Abstract that with a new has_valid_handle() backend operation.
> > >
> > > Make the existing local_fid_fd() helper more robust so that it
> > > can cope with P9_FID_NONE or a null directory stream and reuse
> > > it.
> > >
> > > Signed-off-by: Greg Kurz <gr...@kaod.org>
> > > ---
> > > fsdev/file-op-9p.h | 1 +
> > > hw/9pfs/9p-local.c | 12 +++++++++---
> > > hw/9pfs/9p-synth.c | 6 ++++++
> > > hw/9pfs/9p.c | 9 ++++++---
> > > 4 files changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> > > index 4997677460e8..39fee185f4ce 100644
> > > --- a/fsdev/file-op-9p.h
> > > +++ b/fsdev/file-op-9p.h
> > > @@ -164,6 +164,7 @@ struct FileOperations {
> > > int (*renameat)(FsContext *ctx, V9fsPath *olddir, const char
> > > *old_name,
> > > V9fsPath *newdir, const char *new_name);
> > > int (*unlinkat)(FsContext *ctx, V9fsPath *dir, const char *name, int
> > > flags);
> > > + bool (*has_valid_handle)(int fid_type, V9fsFidOpenState *fs);
> > > };
> > >
> > > #endif
> > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > > index c4366c867988..03e5304ef888 100644
> > > --- a/hw/9pfs/9p-local.c
> > > +++ b/hw/9pfs/9p-local.c
> > > @@ -768,11 +768,11 @@ out:
> > >
> > > static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> > > {
> > > - int fd;
> > > + int fd = -1;
> > >
> > > - if (fid_type == P9_FID_DIR) {
> > > + if (fid_type == P9_FID_DIR && fs->dir.stream != NULL) {
> > > fd = dirfd(fs->dir.stream);
> > > - } else {
> > > + } else if (fid_type == P9_FID_FILE) {
> > > fd = fs->fd;
> > > }
> >
> > Follow-up on previous patch, this could be reduced to:
> >
> > static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> > {
> > if (fid_type == P9_FID_DIR && fs->dir.stream != NULL) {
> > return dirfd(fs->dir.stream);
> > } else if (fid_type == P9_FID_FILE) {
> > return fs->fd;
> > }
> > return -1; /* POSIX invalid file handle */
> > }
> >
> > or even:
> >
> > static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> > {
> > return (fid_type == P9_FID_DIR && fs->dir.stream != NULL) ?
> > dirfd(fs->dir.stream) :
> > (fid_type == P9_FID_FILE) ? fs->fd :
> > -1; /* POSIX invalid file handle */
> > }
> >
>
> Yuck, I'll stick to the `if` version ;-)
>
And this change is no longer needed if I go...
> No sure to understand the meaning of `/* POSIX invalid file handle */`
> though...
>
> > >
> > > @@ -1576,6 +1576,11 @@ static int local_parse_opts(QemuOpts *opts,
> > > FsDriverEntry *fse, Error **errp)
> > > return 0;
> > > }
> > >
> > > +static bool local_has_valid_handle(int fid_type, V9fsFidOpenState *fs)
> > > +{
> > > + return local_fid_fd(fid_type, fs) != -1;
> > > +}
> >
> > I would avoid the implied dirfd() call here. It's usually just a libc
> > function
> > on user side, so usually no context switch, but who knows if that applies to
> > all systems. So adapted to existing code this would be:
> >
> > static bool local_has_valid_handle(int fid_type, V9fsFidOpenState *fs)
> > {
> > return (fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
> > (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream != NULL);
> > }
> >
... that way :-)
> > > FileOperations local_ops = {
> > > .parse_opts = local_parse_opts,
> > > .init = local_init,
> > > @@ -1613,4 +1618,5 @@ FileOperations local_ops = {
> > > .name_to_path = local_name_to_path,
> > > .renameat = local_renameat,
> > > .unlinkat = local_unlinkat,
> > > + .has_valid_handle = local_has_valid_handle,
> > > };
> > > diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> > > index 2abaf3a2918a..fa0b187a1b80 100644
> > > --- a/hw/9pfs/9p-synth.c
> > > +++ b/hw/9pfs/9p-synth.c
> > > @@ -615,6 +615,11 @@ static int synth_init(FsContext *ctx, Error **errp)
> > > return 0;
> > > }
> > >
> > > +static bool synth_has_valid_handle(int fid_type, V9fsFidOpenState *fs)
> > > +{
> > > + return false;
> > > +}
> > > +
> >
> > I was worried that this would cause the synth tests to fail, but apparently
> > it
> > does not break them. So fine.
> >
> > > FileOperations synth_ops = {
> > > .init = synth_init,
> > > .lstat = synth_lstat,
> > > @@ -650,4 +655,5 @@ FileOperations synth_ops = {
> > > .name_to_path = synth_name_to_path,
> > > .renameat = synth_renameat,
> > > .unlinkat = synth_unlinkat,
> > > + .has_valid_handle = synth_has_valid_handle,
> >
> > Mabye rather has_valid_file_handle?
> >
> > > };
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 7cad2bce6209..969fb2f8c494 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -1574,6 +1574,11 @@ out_nofid:
> > > pdu_complete(pdu, err);
> > > }
> > >
> > > +static bool fid_has_valid_handle(V9fsState *s, V9fsFidState *fidp)
> > > +{
> > > + return s->ops->has_valid_handle(fidp->fid_type, &fidp->fs);
> > > +}
> > > +
> > > static void coroutine_fn v9fs_getattr(void *opaque)
> > > {
> > > int32_t fid;
> > > @@ -1596,9 +1601,7 @@ static void coroutine_fn v9fs_getattr(void *opaque)
> > > retval = -ENOENT;
> > > goto out_nofid;
> > > }
> > > - if ((fidp->fid_type == P9_FID_FILE && fidp->fs.fd != -1) ||
> > > - (fidp->fid_type == P9_FID_DIR && fidp->fs.dir.stream))
> > > - {
> > > + if (fid_has_valid_handle(pdu->s, fidp)) {
> > > retval = v9fs_co_fstat(pdu, fidp, &stbuf);
> > > } else {
> > > retval = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> > >
> >
> >
>
>
>
--
Greg
