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 ;-)
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);
> }
>
> > 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
