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 */
}
>
> @@ -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);
>
