* [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper
2025-03-10 17:10 [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink Greg Kurz
@ 2025-03-10 17:10 ` Greg Kurz
2025-03-11 10:58 ` Christian Schoenebeck
2025-03-10 17:10 ` [PATCH 2/4] 9pfs: Don't use file descriptors in core code Greg Kurz
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Greg Kurz @ 2025-03-10 17:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz
Factor out duplicated code to a single helper. More users to come.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p-local.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 928523afcc6c..c4366c867988 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -766,10 +766,9 @@ out:
return err;
}
-static int local_fstat(FsContext *fs_ctx, int fid_type,
- V9fsFidOpenState *fs, struct stat *stbuf)
+static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
- int err, fd;
+ int fd;
if (fid_type == P9_FID_DIR) {
fd = dirfd(fs->dir.stream);
@@ -777,6 +776,14 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
fd = fs->fd;
}
+ return fd;
+}
+
+static int local_fstat(FsContext *fs_ctx, int fid_type,
+ V9fsFidOpenState *fs, struct stat *stbuf)
+{
+ int err, fd = local_fid_fd(fid_type, fs);
+
err = fstat(fd, stbuf);
if (err) {
return err;
@@ -1167,13 +1174,7 @@ out:
static int local_fsync(FsContext *ctx, int fid_type,
V9fsFidOpenState *fs, int datasync)
{
- int fd;
-
- if (fid_type == P9_FID_DIR) {
- fd = dirfd(fs->dir.stream);
- } else {
- fd = fs->fd;
- }
+ int fd = local_fid_fd(fid_type, fs);
if (datasync) {
return qemu_fdatasync(fd);
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper
2025-03-10 17:10 ` [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper Greg Kurz
@ 2025-03-11 10:58 ` Christian Schoenebeck
2025-03-11 14:01 ` Greg Kurz
0 siblings, 1 reply; 14+ messages in thread
From: Christian Schoenebeck @ 2025-03-11 10:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
On Monday, March 10, 2025 6:10:58 PM CET Greg Kurz wrote:
> Factor out duplicated code to a single helper. More users to come.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/9pfs/9p-local.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 928523afcc6c..c4366c867988 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -766,10 +766,9 @@ out:
> return err;
> }
>
> -static int local_fstat(FsContext *fs_ctx, int fid_type,
> - V9fsFidOpenState *fs, struct stat *stbuf)
> +static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> {
> - int err, fd;
> + int fd;
>
> if (fid_type == P9_FID_DIR) {
> fd = dirfd(fs->dir.stream);
> @@ -777,6 +776,14 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
> fd = fs->fd;
> }
>
> + return fd;
> +}
Maybe simplifying this like:
static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
if (fid_type == P9_FID_DIR) {
return dirfd(fs->dir.stream);
} else {
return fs->fd;
}
}
or even just:
static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
{
return (fid_type == P9_FID_DIR) ? dirfd(fs->dir.stream) : return fs->fd;
}
/Christian
> +
> +static int local_fstat(FsContext *fs_ctx, int fid_type,
> + V9fsFidOpenState *fs, struct stat *stbuf)
> +{
> + int err, fd = local_fid_fd(fid_type, fs);
> +
> err = fstat(fd, stbuf);
> if (err) {
> return err;
> @@ -1167,13 +1174,7 @@ out:
> static int local_fsync(FsContext *ctx, int fid_type,
> V9fsFidOpenState *fs, int datasync)
> {
> - int fd;
> -
> - if (fid_type == P9_FID_DIR) {
> - fd = dirfd(fs->dir.stream);
> - } else {
> - fd = fs->fd;
> - }
> + int fd = local_fid_fd(fid_type, fs);
>
> if (datasync) {
> return qemu_fdatasync(fd);
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper
2025-03-11 10:58 ` Christian Schoenebeck
@ 2025-03-11 14:01 ` Greg Kurz
0 siblings, 0 replies; 14+ messages in thread
From: Greg Kurz @ 2025-03-11 14:01 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel
On Tue, 11 Mar 2025 11:58:28 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Monday, March 10, 2025 6:10:58 PM CET Greg Kurz wrote:
> > Factor out duplicated code to a single helper. More users to come.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/9pfs/9p-local.c | 21 +++++++++++----------
> > 1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 928523afcc6c..c4366c867988 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -766,10 +766,9 @@ out:
> > return err;
> > }
> >
> > -static int local_fstat(FsContext *fs_ctx, int fid_type,
> > - V9fsFidOpenState *fs, struct stat *stbuf)
> > +static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> > {
> > - int err, fd;
> > + int fd;
> >
> > if (fid_type == P9_FID_DIR) {
> > fd = dirfd(fs->dir.stream);
> > @@ -777,6 +776,14 @@ static int local_fstat(FsContext *fs_ctx, int fid_type,
> > fd = fs->fd;
> > }
> >
> > + return fd;
> > +}
>
> Maybe simplifying this like:
>
> static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> {
> if (fid_type == P9_FID_DIR) {
> return dirfd(fs->dir.stream);
> } else {
> return fs->fd;
> }
> }
>
> or even just:
>
> static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
> {
> return (fid_type == P9_FID_DIR) ? dirfd(fs->dir.stream) : return fs->fd;
> }
>
I'll go for you suggestion with the `if`. It is clearer than the ternary
expression and it is easier to put a breakpoint. I'm pretty sure all three
result in the same assembly code anyway.
> /Christian
>
> > +
> > +static int local_fstat(FsContext *fs_ctx, int fid_type,
> > + V9fsFidOpenState *fs, struct stat *stbuf)
> > +{
> > + int err, fd = local_fid_fd(fid_type, fs);
> > +
> > err = fstat(fd, stbuf);
> > if (err) {
> > return err;
> > @@ -1167,13 +1174,7 @@ out:
> > static int local_fsync(FsContext *ctx, int fid_type,
> > V9fsFidOpenState *fs, int datasync)
> > {
> > - int fd;
> > -
> > - if (fid_type == P9_FID_DIR) {
> > - fd = dirfd(fs->dir.stream);
> > - } else {
> > - fd = fs->fd;
> > - }
> > + int fd = local_fid_fd(fid_type, fs);
> >
> > if (datasync) {
> > return qemu_fdatasync(fd);
> >
>
>
--
Greg
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] 9pfs: Don't use file descriptors in core code
2025-03-10 17:10 [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink Greg Kurz
2025-03-10 17:10 ` [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper Greg Kurz
@ 2025-03-10 17:10 ` Greg Kurz
2025-03-11 11:13 ` Christian Schoenebeck
2025-03-10 17:11 ` [PATCH 3/4] 9pfs: Introduce ftruncate file op Greg Kurz
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Greg Kurz @ 2025-03-10 17:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz
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 <groug@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;
}
@@ -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;
+}
+
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;
+}
+
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,
};
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);
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] 9pfs: Don't use file descriptors in core code
2025-03-10 17:10 ` [PATCH 2/4] 9pfs: Don't use file descriptors in core code Greg Kurz
@ 2025-03-11 11:13 ` Christian Schoenebeck
2025-03-11 14:03 ` Greg Kurz
0 siblings, 1 reply; 14+ messages in thread
From: Christian Schoenebeck @ 2025-03-11 11:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
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 <groug@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);
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] 9pfs: Don't use file descriptors in core code
2025-03-11 11:13 ` Christian Schoenebeck
@ 2025-03-11 14:03 ` Greg Kurz
2025-03-11 14:13 ` Greg Kurz
2025-03-11 14:18 ` Christian Schoenebeck
0 siblings, 2 replies; 14+ messages in thread
From: Greg Kurz @ 2025-03-11 14:03 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel
On Tue, 11 Mar 2025 12:13:06 +0100
Christian Schoenebeck <qemu_oss@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 <groug@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] 9pfs: Don't use file descriptors in core code
2025-03-11 14:03 ` Greg Kurz
@ 2025-03-11 14:13 ` Greg Kurz
2025-03-11 14:18 ` Christian Schoenebeck
1 sibling, 0 replies; 14+ messages in thread
From: Greg Kurz @ 2025-03-11 14:13 UTC (permalink / raw)
To: Christian Schoenebeck; +Cc: qemu-devel
On Tue, 11 Mar 2025 15:03:13 +0100
Greg Kurz <groug@kaod.org> wrote:
> On Tue, 11 Mar 2025 12:13:06 +0100
> Christian Schoenebeck <qemu_oss@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 <groug@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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] 9pfs: Don't use file descriptors in core code
2025-03-11 14:03 ` Greg Kurz
2025-03-11 14:13 ` Greg Kurz
@ 2025-03-11 14:18 ` Christian Schoenebeck
1 sibling, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-03-11 14:18 UTC (permalink / raw)
To: qemu-devel, Greg Kurz
On Tuesday, March 11, 2025 3:03:13 PM CET Greg Kurz wrote:
> On Tue, 11 Mar 2025 12:13:06 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>
> > On Monday, March 10, 2025 6:10:59 PM CET Greg Kurz wrote:
[...]
> > 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...
"invalid file descriptor" rather than "invalid file handle". What I mean is
that outside of the Unix world -1 is not necessarily the predefined and
reserved value for "invalid file descriptor".
But no big deal.
/Christian
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] 9pfs: Introduce ftruncate file op
2025-03-10 17:10 [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink Greg Kurz
2025-03-10 17:10 ` [PATCH 1/4] 9pfs: local : Introduce local_fid_fd() helper Greg Kurz
2025-03-10 17:10 ` [PATCH 2/4] 9pfs: Don't use file descriptors in core code Greg Kurz
@ 2025-03-10 17:11 ` Greg Kurz
2025-03-11 11:19 ` Christian Schoenebeck
2025-03-10 17:11 ` [PATCH 4/4] 9pfs: Introduce futimens " Greg Kurz
2025-03-11 10:51 ` [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink Christian Schoenebeck
4 siblings, 1 reply; 14+ messages in thread
From: Greg Kurz @ 2025-03-10 17:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz
Add an ftruncate operation to the fs driver and use if when a fid has
a valid file descriptor. This is required to support more cases where
the client wants to do an action on an unlinked file which it still
has an open file decriptor for.
Only 9P2000.L was considered.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
fsdev/file-op-9p.h | 2 ++
hw/9pfs/9p-local.c | 9 +++++++++
hw/9pfs/9p-synth.c | 8 ++++++++
hw/9pfs/9p.c | 6 +++++-
hw/9pfs/cofs.c | 18 ++++++++++++++++++
hw/9pfs/coth.h | 2 ++
6 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 39fee185f4ce..40a40f7d8af8 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -165,6 +165,8 @@ struct FileOperations {
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);
+ int (*ftruncate)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ off_t size);
};
#endif
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 03e5304ef888..fa763b0662f5 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1046,6 +1046,14 @@ static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
return ret;
}
+static int local_ftruncate(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ off_t size)
+{
+ int fd = local_fid_fd(fid_type, fs);
+
+ return ftruncate(fd, size);
+}
+
static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
{
char *dirpath = g_path_get_dirname(fs_path->data);
@@ -1619,4 +1627,5 @@ FileOperations local_ops = {
.renameat = local_renameat,
.unlinkat = local_unlinkat,
.has_valid_handle = local_has_valid_handle,
+ .ftruncate = local_ftruncate,
};
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index fa0b187a1b80..7f0e13f0ddd8 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -356,6 +356,13 @@ static int synth_truncate(FsContext *ctx, V9fsPath *path, off_t offset)
return -1;
}
+static int synth_ftruncate(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ off_t size)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
static int synth_chmod(FsContext *fs_ctx, V9fsPath *path, FsCred *credp)
{
errno = EPERM;
@@ -656,4 +663,5 @@ FileOperations synth_ops = {
.renameat = synth_renameat,
.unlinkat = synth_unlinkat,
.has_valid_handle = synth_has_valid_handle,
+ .ftruncate = synth_ftruncate,
};
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 969fb2f8c494..35b2ed900a01 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1733,7 +1733,11 @@ static void coroutine_fn v9fs_setattr(void *opaque)
}
}
if (v9iattr.valid & (P9_ATTR_SIZE)) {
- err = v9fs_co_truncate(pdu, &fidp->path, v9iattr.size);
+ if (fid_has_valid_handle(pdu->s, fidp)) {
+ err = v9fs_co_ftruncate(pdu, fidp, v9iattr.size);
+ } else {
+ err = v9fs_co_truncate(pdu, &fidp->path, v9iattr.size);
+ }
if (err < 0) {
goto out;
}
diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
index 67e3ae5c5ccd..893466fb1a44 100644
--- a/hw/9pfs/cofs.c
+++ b/hw/9pfs/cofs.c
@@ -184,6 +184,24 @@ int coroutine_fn v9fs_co_truncate(V9fsPDU *pdu, V9fsPath *path, off_t size)
return err;
}
+int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp, off_t size)
+{
+ int err;
+ V9fsState *s = pdu->s;
+
+ if (v9fs_request_cancelled(pdu)) {
+ return -EINTR;
+ }
+ v9fs_co_run_in_worker(
+ {
+ err = s->ops->ftruncate(&s->ctx, fidp->fid_type, &fidp->fs, size);
+ if (err < 0) {
+ err = -errno;
+ }
+ });
+ return err;
+}
+
int coroutine_fn v9fs_co_mknod(V9fsPDU *pdu, V9fsFidState *fidp,
V9fsString *name, uid_t uid, gid_t gid,
dev_t dev, mode_t mode, struct stat *stbuf)
diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
index 2c54249b3577..0b8ee4c56495 100644
--- a/hw/9pfs/coth.h
+++ b/hw/9pfs/coth.h
@@ -109,5 +109,7 @@ int coroutine_fn v9fs_co_name_to_path(V9fsPDU *, V9fsPath *,
const char *, V9fsPath *);
int coroutine_fn v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t,
V9fsStatDotl *v9stat);
+int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp,
+ off_t size);
#endif
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/4] 9pfs: Introduce ftruncate file op
2025-03-10 17:11 ` [PATCH 3/4] 9pfs: Introduce ftruncate file op Greg Kurz
@ 2025-03-11 11:19 ` Christian Schoenebeck
0 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-03-11 11:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
On Monday, March 10, 2025 6:11:00 PM CET Greg Kurz wrote:
> Add an ftruncate operation to the fs driver and use if when a fid has
> a valid file descriptor. This is required to support more cases where
> the client wants to do an action on an unlinked file which it still
> has an open file decriptor for.
>
> Only 9P2000.L was considered.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> fsdev/file-op-9p.h | 2 ++
> hw/9pfs/9p-local.c | 9 +++++++++
> hw/9pfs/9p-synth.c | 8 ++++++++
> hw/9pfs/9p.c | 6 +++++-
> hw/9pfs/cofs.c | 18 ++++++++++++++++++
> hw/9pfs/coth.h | 2 ++
> 6 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 39fee185f4ce..40a40f7d8af8 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -165,6 +165,8 @@ struct FileOperations {
> 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);
> + int (*ftruncate)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
> + off_t size);
> };
>
> #endif
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 03e5304ef888..fa763b0662f5 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1046,6 +1046,14 @@ static int local_truncate(FsContext *ctx, V9fsPath *fs_path, off_t size)
> return ret;
> }
>
> +static int local_ftruncate(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
> + off_t size)
> +{
> + int fd = local_fid_fd(fid_type, fs);
> +
> + return ftruncate(fd, size);
> +}
> +
> static int local_chown(FsContext *fs_ctx, V9fsPath *fs_path, FsCred *credp)
> {
> char *dirpath = g_path_get_dirname(fs_path->data);
> @@ -1619,4 +1627,5 @@ FileOperations local_ops = {
> .renameat = local_renameat,
> .unlinkat = local_unlinkat,
> .has_valid_handle = local_has_valid_handle,
> + .ftruncate = local_ftruncate,
> };
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index fa0b187a1b80..7f0e13f0ddd8 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -356,6 +356,13 @@ static int synth_truncate(FsContext *ctx, V9fsPath *path, off_t offset)
> return -1;
> }
>
> +static int synth_ftruncate(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
> + off_t size)
> +{
> + errno = ENOSYS;
> + return -1;
> +}
> +
> static int synth_chmod(FsContext *fs_ctx, V9fsPath *path, FsCred *credp)
> {
> errno = EPERM;
> @@ -656,4 +663,5 @@ FileOperations synth_ops = {
> .renameat = synth_renameat,
> .unlinkat = synth_unlinkat,
> .has_valid_handle = synth_has_valid_handle,
> + .ftruncate = synth_ftruncate,
> };
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 969fb2f8c494..35b2ed900a01 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1733,7 +1733,11 @@ static void coroutine_fn v9fs_setattr(void *opaque)
> }
> }
> if (v9iattr.valid & (P9_ATTR_SIZE)) {
> - err = v9fs_co_truncate(pdu, &fidp->path, v9iattr.size);
> + if (fid_has_valid_handle(pdu->s, fidp)) {
> + err = v9fs_co_ftruncate(pdu, fidp, v9iattr.size);
> + } else {
> + err = v9fs_co_truncate(pdu, &fidp->path, v9iattr.size);
> + }
> if (err < 0) {
> goto out;
> }
> diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
> index 67e3ae5c5ccd..893466fb1a44 100644
> --- a/hw/9pfs/cofs.c
> +++ b/hw/9pfs/cofs.c
> @@ -184,6 +184,24 @@ int coroutine_fn v9fs_co_truncate(V9fsPDU *pdu, V9fsPath *path, off_t size)
> return err;
> }
>
> +int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp, off_t size)
> +{
> + int err;
> + V9fsState *s = pdu->s;
> +
> + if (v9fs_request_cancelled(pdu)) {
> + return -EINTR;
> + }
> + v9fs_co_run_in_worker(
> + {
> + err = s->ops->ftruncate(&s->ctx, fidp->fid_type, &fidp->fs, size);
> + if (err < 0) {
> + err = -errno;
> + }
> + });
> + return err;
> +}
> +
> int coroutine_fn v9fs_co_mknod(V9fsPDU *pdu, V9fsFidState *fidp,
> V9fsString *name, uid_t uid, gid_t gid,
> dev_t dev, mode_t mode, struct stat *stbuf)
> diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
> index 2c54249b3577..0b8ee4c56495 100644
> --- a/hw/9pfs/coth.h
> +++ b/hw/9pfs/coth.h
> @@ -109,5 +109,7 @@ int coroutine_fn v9fs_co_name_to_path(V9fsPDU *, V9fsPath *,
> const char *, V9fsPath *);
> int coroutine_fn v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t,
> V9fsStatDotl *v9stat);
> +int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp,
> + off_t size);
>
> #endif
Nit: I would move v9fs_co_ftruncate() close to v9fs_co_truncate() to make it
easier preserving the overview on this header file.
Rest looks fine:
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
/Christian
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] 9pfs: Introduce futimens file op
2025-03-10 17:10 [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink Greg Kurz
` (2 preceding siblings ...)
2025-03-10 17:11 ` [PATCH 3/4] 9pfs: Introduce ftruncate file op Greg Kurz
@ 2025-03-10 17:11 ` Greg Kurz
2025-03-11 11:25 ` Christian Schoenebeck
2025-03-11 10:51 ` [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink Christian Schoenebeck
4 siblings, 1 reply; 14+ messages in thread
From: Greg Kurz @ 2025-03-10 17:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Christian Schoenebeck, Greg Kurz
Add an futimens operation to the fs driver and use if when a fid has
a valid file descriptor. This is required to support more cases where
the client wants to do an action on an unlinked file which it still
has an open file decriptor for.
Only 9P2000.L was considered.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
fsdev/file-op-9p.h | 2 ++
hw/9pfs/9p-local.c | 9 +++++++++
hw/9pfs/9p-synth.c | 8 ++++++++
hw/9pfs/9p-util.h | 1 +
hw/9pfs/9p.c | 6 +++++-
hw/9pfs/cofs.c | 19 +++++++++++++++++++
hw/9pfs/coth.h | 2 ++
7 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 40a40f7d8af8..ee65f4ffad2f 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -167,6 +167,8 @@ struct FileOperations {
bool (*has_valid_handle)(int fid_type, V9fsFidOpenState *fs);
int (*ftruncate)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
off_t size);
+ int (*futimens)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
+ const struct timespec *times);
};
#endif
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index fa763b0662f5..f8b932802c17 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1104,6 +1104,14 @@ out:
return ret;
}
+static int local_futimens(FsContext *s, int fid_type, V9fsFidOpenState *fs,
+ const struct timespec *times)
+{
+ int fd = local_fid_fd(fid_type, fs);
+
+ return qemu_futimens(fd, times);
+}
+
static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
int flags)
{
@@ -1628,4 +1636,5 @@ FileOperations local_ops = {
.unlinkat = local_unlinkat,
.has_valid_handle = local_has_valid_handle,
.ftruncate = local_ftruncate,
+ .futimens = local_futimens,
};
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 7f0e13f0ddd8..3bf4bb17e767 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -424,6 +424,13 @@ static int synth_utimensat(FsContext *fs_ctx, V9fsPath *path,
return 0;
}
+static int synth_futimens(FsContext *fs_ctx, int fid_type, V9fsFidOpenState *fs,
+ const struct timespec *buf)
+{
+ errno = ENOSYS;
+ return -1;
+}
+
static int synth_remove(FsContext *ctx, const char *path)
{
errno = EPERM;
@@ -664,4 +671,5 @@ FileOperations synth_ops = {
.unlinkat = synth_unlinkat,
.has_valid_handle = synth_has_valid_handle,
.ftruncate = synth_ftruncate,
+ .futimens = synth_futimens,
};
diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
index 7bc4ec8e85cc..a1924fe3f05a 100644
--- a/hw/9pfs/9p-util.h
+++ b/hw/9pfs/9p-util.h
@@ -103,6 +103,7 @@ static inline int errno_to_dotl(int err) {
#define qemu_renameat renameat
#define qemu_utimensat utimensat
#define qemu_unlinkat unlinkat
+#define qemu_futimens futimens
static inline void close_preserve_errno(int fd)
{
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 35b2ed900a01..8024cc06a090 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1708,7 +1708,11 @@ static void coroutine_fn v9fs_setattr(void *opaque)
} else {
times[1].tv_nsec = UTIME_OMIT;
}
- err = v9fs_co_utimensat(pdu, &fidp->path, times);
+ if (fid_has_valid_handle(pdu->s, fidp)) {
+ err = v9fs_co_futimens(pdu, fidp, times);
+ } else {
+ err = v9fs_co_utimensat(pdu, &fidp->path, times);
+ }
if (err < 0) {
goto out;
}
diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
index 893466fb1a44..12fa8c9fe9cd 100644
--- a/hw/9pfs/cofs.c
+++ b/hw/9pfs/cofs.c
@@ -139,6 +139,25 @@ int coroutine_fn v9fs_co_utimensat(V9fsPDU *pdu, V9fsPath *path,
return err;
}
+int coroutine_fn v9fs_co_futimens(V9fsPDU *pdu, V9fsFidState *fidp,
+ struct timespec times[2])
+{
+ int err;
+ V9fsState *s = pdu->s;
+
+ if (v9fs_request_cancelled(pdu)) {
+ return -EINTR;
+ }
+ v9fs_co_run_in_worker(
+ {
+ err = s->ops->futimens(&s->ctx, fidp->fid_type, &fidp->fs, times);
+ if (err < 0) {
+ err = -errno;
+ }
+ });
+ return err;
+}
+
int coroutine_fn v9fs_co_chown(V9fsPDU *pdu, V9fsPath *path, uid_t uid,
gid_t gid)
{
diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
index 0b8ee4c56495..9920f98129e9 100644
--- a/hw/9pfs/coth.h
+++ b/hw/9pfs/coth.h
@@ -111,5 +111,7 @@ int coroutine_fn v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t,
V9fsStatDotl *v9stat);
int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp,
off_t size);
+int coroutine_fn v9fs_co_futimens(V9fsPDU *pdu, V9fsFidState *fidp,
+ struct timespec times[2]);
#endif
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] 9pfs: Introduce futimens file op
2025-03-10 17:11 ` [PATCH 4/4] 9pfs: Introduce futimens " Greg Kurz
@ 2025-03-11 11:25 ` Christian Schoenebeck
0 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-03-11 11:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
On Monday, March 10, 2025 6:11:01 PM CET Greg Kurz wrote:
> Add an futimens operation to the fs driver and use if when a fid has
> a valid file descriptor. This is required to support more cases where
> the client wants to do an action on an unlinked file which it still
> has an open file decriptor for.
>
> Only 9P2000.L was considered.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> fsdev/file-op-9p.h | 2 ++
> hw/9pfs/9p-local.c | 9 +++++++++
> hw/9pfs/9p-synth.c | 8 ++++++++
> hw/9pfs/9p-util.h | 1 +
> hw/9pfs/9p.c | 6 +++++-
> hw/9pfs/cofs.c | 19 +++++++++++++++++++
> hw/9pfs/coth.h | 2 ++
> 7 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 40a40f7d8af8..ee65f4ffad2f 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -167,6 +167,8 @@ struct FileOperations {
> bool (*has_valid_handle)(int fid_type, V9fsFidOpenState *fs);
> int (*ftruncate)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
> off_t size);
> + int (*futimens)(FsContext *ctx, int fid_type, V9fsFidOpenState *fs,
> + const struct timespec *times);
> };
>
> #endif
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index fa763b0662f5..f8b932802c17 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1104,6 +1104,14 @@ out:
> return ret;
> }
>
> +static int local_futimens(FsContext *s, int fid_type, V9fsFidOpenState *fs,
> + const struct timespec *times)
> +{
> + int fd = local_fid_fd(fid_type, fs);
> +
> + return qemu_futimens(fd, times);
> +}
> +
> static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
> int flags)
> {
> @@ -1628,4 +1636,5 @@ FileOperations local_ops = {
> .unlinkat = local_unlinkat,
> .has_valid_handle = local_has_valid_handle,
> .ftruncate = local_ftruncate,
> + .futimens = local_futimens,
> };
> diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
> index 7f0e13f0ddd8..3bf4bb17e767 100644
> --- a/hw/9pfs/9p-synth.c
> +++ b/hw/9pfs/9p-synth.c
> @@ -424,6 +424,13 @@ static int synth_utimensat(FsContext *fs_ctx, V9fsPath *path,
> return 0;
> }
>
> +static int synth_futimens(FsContext *fs_ctx, int fid_type, V9fsFidOpenState *fs,
> + const struct timespec *buf)
> +{
> + errno = ENOSYS;
> + return -1;
> +}
> +
> static int synth_remove(FsContext *ctx, const char *path)
> {
> errno = EPERM;
> @@ -664,4 +671,5 @@ FileOperations synth_ops = {
> .unlinkat = synth_unlinkat,
> .has_valid_handle = synth_has_valid_handle,
> .ftruncate = synth_ftruncate,
> + .futimens = synth_futimens,
> };
> diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h
> index 7bc4ec8e85cc..a1924fe3f05a 100644
> --- a/hw/9pfs/9p-util.h
> +++ b/hw/9pfs/9p-util.h
> @@ -103,6 +103,7 @@ static inline int errno_to_dotl(int err) {
> #define qemu_renameat renameat
> #define qemu_utimensat utimensat
> #define qemu_unlinkat unlinkat
> +#define qemu_futimens futimens
>
> static inline void close_preserve_errno(int fd)
> {
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 35b2ed900a01..8024cc06a090 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1708,7 +1708,11 @@ static void coroutine_fn v9fs_setattr(void *opaque)
> } else {
> times[1].tv_nsec = UTIME_OMIT;
> }
> - err = v9fs_co_utimensat(pdu, &fidp->path, times);
> + if (fid_has_valid_handle(pdu->s, fidp)) {
> + err = v9fs_co_futimens(pdu, fidp, times);
> + } else {
> + err = v9fs_co_utimensat(pdu, &fidp->path, times);
> + }
> if (err < 0) {
> goto out;
> }
> diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
> index 893466fb1a44..12fa8c9fe9cd 100644
> --- a/hw/9pfs/cofs.c
> +++ b/hw/9pfs/cofs.c
> @@ -139,6 +139,25 @@ int coroutine_fn v9fs_co_utimensat(V9fsPDU *pdu, V9fsPath *path,
> return err;
> }
>
> +int coroutine_fn v9fs_co_futimens(V9fsPDU *pdu, V9fsFidState *fidp,
> + struct timespec times[2])
> +{
> + int err;
> + V9fsState *s = pdu->s;
> +
> + if (v9fs_request_cancelled(pdu)) {
> + return -EINTR;
> + }
> + v9fs_co_run_in_worker(
> + {
> + err = s->ops->futimens(&s->ctx, fidp->fid_type, &fidp->fs, times);
> + if (err < 0) {
> + err = -errno;
> + }
> + });
> + return err;
> +}
> +
> int coroutine_fn v9fs_co_chown(V9fsPDU *pdu, V9fsPath *path, uid_t uid,
> gid_t gid)
> {
> diff --git a/hw/9pfs/coth.h b/hw/9pfs/coth.h
> index 0b8ee4c56495..9920f98129e9 100644
> --- a/hw/9pfs/coth.h
> +++ b/hw/9pfs/coth.h
> @@ -111,5 +111,7 @@ int coroutine_fn v9fs_co_st_gen(V9fsPDU *pdu, V9fsPath *path, mode_t,
> V9fsStatDotl *v9stat);
> int coroutine_fn v9fs_co_ftruncate(V9fsPDU *pdu, V9fsFidState *fidp,
> off_t size);
> +int coroutine_fn v9fs_co_futimens(V9fsPDU *pdu, V9fsFidState *fidp,
> + struct timespec times[2]);
>
> #endif
Same nit as with previous patch: I would group v9fs_co_futimens() and
v9fs_co_utimensat() together in this header file for overview reason.
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
/Christian
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink
2025-03-10 17:10 [PATCH 0/4] 9pfs: Fix ftruncate-after-unlink Greg Kurz
` (3 preceding siblings ...)
2025-03-10 17:11 ` [PATCH 4/4] 9pfs: Introduce futimens " Greg Kurz
@ 2025-03-11 10:51 ` Christian Schoenebeck
4 siblings, 0 replies; 14+ messages in thread
From: Christian Schoenebeck @ 2025-03-11 10:51 UTC (permalink / raw)
To: qemu-devel; +Cc: Greg Kurz
On Monday, March 10, 2025 6:10:57 PM CET Greg Kurz wrote:
> QEMU 9.2 already fixed the long standing limitation of failing fstat() on
> unlinked files. This series does something similar for ftruncate().
>
> The following program can be straced inside the guest with a shared fs in
> passthrough mode over 9p2000.L.
>
> int main(void)
> {
> struct stat st;
> int fd = creat("./foo", 0000);
>
> ftruncate(fd, 100);
> unlink("./foo");
> ftruncate(fd, 1000);
> }
>
> Before :
>
> creat("./foo", 000) = 3
> ftruncate(3, 100) = -1 EACCES (Permission denied)
> unlink("./foo") = 0
> ftruncate(3, 1000) = -1 ENOENT (No such file or directory)
>
> After :
>
> creat("./foo", 000) = 3
> ftruncate(3, 100) = 0
> unlink("./foo") = 0
> ftruncate(3, 1000) = 0
>
> Christian,
>
> I'm not familiar enough with the latest changes to write a proper test
> for this case and I don't have enough cycles to learn. I'm sorry for that
> but I guess it will be a lot easier for you and I'll review.
With the following test client patch applied:
https://lore.kernel.org/all/E1trx7R-002JEJ-0l@kylie.crudebyte.com/
then something like this should do it:
diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
index f515a9bb15..d15721e4b2 100644
--- a/tests/qtest/virtio-9p-test.c
+++ b/tests/qtest/virtio-9p-test.c
@@ -736,6 +736,12 @@ static void fs_use_after_unlink(void *obj, void *data,
.data = buf
}).count;
g_assert_cmpint(count, ==, write_count);
+ tsetattr({ /* truncate file to (arbitrarily chosen) size 2001 */
+ .client = v9p, .fid = fid_file, .attr = (v9fs_attr) {
+ .valid = P9_SETATTR_SIZE,
+ .size = 2001
+ }
+ });
}
static void cleanup_9p_local_driver(void *data)
/Christian
> Cheers,
>
> --
> Greg
>
> Greg Kurz (4):
> 9pfs: local : Introduce local_fid_fd() helper
> 9pfs: Don't use file descriptors in core code
> 9pfs: Introduce ftruncate file op
> 9pfs: Introduce futimens file op
>
> fsdev/file-op-9p.h | 5 +++++
> hw/9pfs/9p-local.c | 49 ++++++++++++++++++++++++++++++++++------------
> hw/9pfs/9p-synth.c | 22 +++++++++++++++++++++
> hw/9pfs/9p-util.h | 1 +
> hw/9pfs/9p.c | 21 +++++++++++++++-----
> hw/9pfs/cofs.c | 37 ++++++++++++++++++++++++++++++++++
> hw/9pfs/coth.h | 4 ++++
> 7 files changed, 122 insertions(+), 17 deletions(-)
>
>
^ permalink raw reply related [flat|nested] 14+ messages in thread