* [PULL 1/5] 9pfs/proxy: Check return value of proxy_marshal()
2021-01-25 11:44 [PULL 0/5] 9p next patches Greg Kurz
@ 2021-01-25 11:44 ` Greg Kurz
2021-01-25 11:44 ` [PULL 2/5] 9pfs: Convert V9fsFidState::clunked to bool Greg Kurz
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Greg Kurz @ 2021-01-25 11:44 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Christian Schoenebeck, Greg Kurz
This should always successfully write exactly two 32-bit integers.
Make it clear with an assert(), like v9fs_receive_status() and
v9fs_receive_response() already do when unmarshalling the same
header.
Fixes: Coverity CID 1438968
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <161035859647.1221144.4691749806675653934.stgit@bahia.lan>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p-proxy.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 6f598a0f111c..4aa4e0a3baa0 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -537,7 +537,8 @@ static int v9fs_request(V9fsProxy *proxy, int type, void *response, ...)
}
/* marshal the header details */
- proxy_marshal(iovec, 0, "dd", header.type, header.size);
+ retval = proxy_marshal(iovec, 0, "dd", header.type, header.size);
+ assert(retval == 4 * 2);
header.size += PROXY_HDR_SZ;
retval = qemu_write_full(proxy->sockfd, iovec->iov_base, header.size);
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PULL 2/5] 9pfs: Convert V9fsFidState::clunked to bool
2021-01-25 11:44 [PULL 0/5] 9p next patches Greg Kurz
2021-01-25 11:44 ` [PULL 1/5] 9pfs/proxy: Check return value of proxy_marshal() Greg Kurz
@ 2021-01-25 11:44 ` Greg Kurz
2021-01-25 11:44 ` [PULL 3/5] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ Greg Kurz
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Greg Kurz @ 2021-01-25 11:44 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Christian Schoenebeck, Greg Kurz
This can only be 0 or 1.
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20210118142300.801516-2-groug@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 4 ++--
hw/9pfs/9p.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 6026b51a1c04..37c3379b7462 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -413,7 +413,7 @@ static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
}
fidp = *fidpp;
*fidpp = fidp->next;
- fidp->clunked = 1;
+ fidp->clunked = true;
return fidp;
}
@@ -544,7 +544,7 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
/* Clunk fid */
s->fid_list = fidp->next;
- fidp->clunked = 1;
+ fidp->clunked = true;
put_fid(pdu, fidp);
}
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 32df81f360ea..93656323d1d7 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -279,7 +279,7 @@ struct V9fsFidState {
int open_flags;
uid_t uid;
int ref;
- int clunked;
+ bool clunked;
V9fsFidState *next;
V9fsFidState *rclm_lst;
};
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PULL 3/5] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ
2021-01-25 11:44 [PULL 0/5] 9p next patches Greg Kurz
2021-01-25 11:44 ` [PULL 1/5] 9pfs/proxy: Check return value of proxy_marshal() Greg Kurz
2021-01-25 11:44 ` [PULL 2/5] 9pfs: Convert V9fsFidState::clunked to bool Greg Kurz
@ 2021-01-25 11:44 ` Greg Kurz
2021-01-25 11:44 ` [PULL 4/5] 9pfs: Improve unreclaim loop Greg Kurz
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Greg Kurz @ 2021-01-25 11:44 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Christian Schoenebeck, Greg Kurz
The fid_list is currently open-coded. This doesn't seem to serve any
purpose that cannot be met with QEMU's generic lists. Let's go for a
QSIMPLEQ : this will allow to add new fids at the end of the list and
to improve the logic in v9fs_mark_fids_unreclaim().
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20210118142300.801516-3-groug@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 41 ++++++++++++++++++-----------------------
hw/9pfs/9p.h | 4 ++--
2 files changed, 20 insertions(+), 25 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 37c3379b7462..b65f320e6518 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -260,7 +260,7 @@ static V9fsFidState *coroutine_fn get_fid(V9fsPDU *pdu, int32_t fid)
V9fsFidState *f;
V9fsState *s = pdu->s;
- for (f = s->fid_list; f; f = f->next) {
+ QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
BUG_ON(f->clunked);
if (f->fid == fid) {
/*
@@ -295,7 +295,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
{
V9fsFidState *f;
- for (f = s->fid_list; f; f = f->next) {
+ QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
/* If fid is already there return NULL */
BUG_ON(f->clunked);
if (f->fid == fid) {
@@ -311,8 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
* reclaim won't close the file descriptor
*/
f->flags |= FID_REFERENCED;
- f->next = s->fid_list;
- s->fid_list = f;
+ QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
v9fs_readdir_init(s->proto_version, &f->fs.dir);
v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
@@ -401,20 +400,16 @@ static int coroutine_fn put_fid(V9fsPDU *pdu, V9fsFidState *fidp)
static V9fsFidState *clunk_fid(V9fsState *s, int32_t fid)
{
- V9fsFidState **fidpp, *fidp;
+ V9fsFidState *fidp;
- for (fidpp = &s->fid_list; *fidpp; fidpp = &(*fidpp)->next) {
- if ((*fidpp)->fid == fid) {
- break;
+ QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
+ if (fidp->fid == fid) {
+ QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
+ fidp->clunked = true;
+ return fidp;
}
}
- if (*fidpp == NULL) {
- return NULL;
- }
- fidp = *fidpp;
- *fidpp = fidp->next;
- fidp->clunked = true;
- return fidp;
+ return NULL;
}
void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
@@ -423,7 +418,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
V9fsState *s = pdu->s;
V9fsFidState *f, *reclaim_list = NULL;
- for (f = s->fid_list; f; f = f->next) {
+ QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
/*
* Unlink fids cannot be reclaimed. Check
* for them and skip them. Also skip fids
@@ -505,7 +500,7 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path)
V9fsFidState *fidp;
again:
- for (fidp = s->fid_list; fidp; fidp = fidp->next) {
+ QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
if (fidp->path.size != path->size) {
continue;
}
@@ -537,13 +532,13 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
V9fsFidState *fidp;
/* Free all fids */
- while (s->fid_list) {
+ while (!QSIMPLEQ_EMPTY(&s->fid_list)) {
/* Get fid */
- fidp = s->fid_list;
+ fidp = QSIMPLEQ_FIRST(&s->fid_list);
fidp->ref++;
/* Clunk fid */
- s->fid_list = fidp->next;
+ QSIMPLEQ_REMOVE(&s->fid_list, fidp, V9fsFidState, next);
fidp->clunked = true;
put_fid(pdu, fidp);
@@ -3121,7 +3116,7 @@ static int coroutine_fn v9fs_complete_rename(V9fsPDU *pdu, V9fsFidState *fidp,
* Fixup fid's pointing to the old name to
* start pointing to the new name
*/
- for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
+ QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
if (v9fs_path_is_ancestor(&fidp->path, &tfidp->path)) {
/* replace the name */
v9fs_fix_path(&tfidp->path, &new_path, strlen(fidp->path.data));
@@ -3215,7 +3210,7 @@ static int coroutine_fn v9fs_fix_fid_paths(V9fsPDU *pdu, V9fsPath *olddir,
* Fixup fid's pointing to the old name to
* start pointing to the new name
*/
- for (tfidp = s->fid_list; tfidp; tfidp = tfidp->next) {
+ QSIMPLEQ_FOREACH(tfidp, &s->fid_list, next) {
if (v9fs_path_is_ancestor(&oldpath, &tfidp->path)) {
/* replace the name */
v9fs_fix_path(&tfidp->path, &newpath, strlen(oldpath.data));
@@ -4081,7 +4076,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
s->ctx.fmode = fse->fmode;
s->ctx.dmode = fse->dmode;
- s->fid_list = NULL;
+ QSIMPLEQ_INIT(&s->fid_list);
qemu_co_rwlock_init(&s->rename_lock);
if (s->ops->init(&s->ctx, errp) < 0) {
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 93656323d1d7..85fb6930b0ca 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -280,7 +280,7 @@ struct V9fsFidState {
uid_t uid;
int ref;
bool clunked;
- V9fsFidState *next;
+ QSIMPLEQ_ENTRY(V9fsFidState) next;
V9fsFidState *rclm_lst;
};
@@ -339,7 +339,7 @@ typedef struct {
struct V9fsState {
QLIST_HEAD(, V9fsPDU) free_list;
QLIST_HEAD(, V9fsPDU) active_list;
- V9fsFidState *fid_list;
+ QSIMPLEQ_HEAD(, V9fsFidState) fid_list;
FileOperations *ops;
FsContext ctx;
char *tag;
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PULL 4/5] 9pfs: Improve unreclaim loop
2021-01-25 11:44 [PULL 0/5] 9p next patches Greg Kurz
` (2 preceding siblings ...)
2021-01-25 11:44 ` [PULL 3/5] 9pfs: Convert V9fsFidState::fid_list to QSIMPLEQ Greg Kurz
@ 2021-01-25 11:44 ` Greg Kurz
2021-01-25 11:44 ` [PULL 5/5] 9pfs: Convert reclaim list to QSLIST Greg Kurz
2021-01-25 15:56 ` [PULL 0/5] 9p next patches Peter Maydell
5 siblings, 0 replies; 7+ messages in thread
From: Greg Kurz @ 2021-01-25 11:44 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Christian Schoenebeck, Greg Kurz
If a fid was actually re-opened by v9fs_reopen_fid(), we re-traverse the
fid list from the head in case some other request created a fid that
needs to be marked unreclaimable as well (i.e. the client opened a new
handle on the path that is being unlinked). This is suboptimal since
most if not all fids that require it have likely been taken care of
already.
This is mostly the result of new fids being added to the head of the
list. Since the list is now a QSIMPLEQ, add new fids at the end instead
to avoid the need to rewind. Take a reference on the fid to ensure it
doesn't go away during v9fs_reopen_fid() and that it can be safely
passed to QSIMPLEQ_NEXT() afterwards. Since the associated put_fid()
can also yield, same is done with the next fid. So the logic here is
to get a reference on a fid and only put it back during the next
iteration after we could get a reference on the next fid.
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20210121181510.1459390-1-groug@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 46 ++++++++++++++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b65f320e6518..3864d014b43c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -311,7 +311,7 @@ static V9fsFidState *alloc_fid(V9fsState *s, int32_t fid)
* reclaim won't close the file descriptor
*/
f->flags |= FID_REFERENCED;
- QSIMPLEQ_INSERT_HEAD(&s->fid_list, f, next);
+ QSIMPLEQ_INSERT_TAIL(&s->fid_list, f, next);
v9fs_readdir_init(s->proto_version, &f->fs.dir);
v9fs_readdir_init(s->proto_version, &f->fs_reclaim.dir);
@@ -497,32 +497,50 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path)
{
int err;
V9fsState *s = pdu->s;
- V9fsFidState *fidp;
+ V9fsFidState *fidp, *fidp_next;
-again:
- QSIMPLEQ_FOREACH(fidp, &s->fid_list, next) {
- if (fidp->path.size != path->size) {
- continue;
- }
- if (!memcmp(fidp->path.data, path->data, path->size)) {
+ fidp = QSIMPLEQ_FIRST(&s->fid_list);
+ if (!fidp) {
+ return 0;
+ }
+
+ /*
+ * v9fs_reopen_fid() can yield : a reference on the fid must be held
+ * to ensure its pointer remains valid and we can safely pass it to
+ * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
+ * we must keep a reference on the next fid as well. So the logic here
+ * is to get a reference on a fid and only put it back during the next
+ * iteration after we could get a reference on the next fid. Start with
+ * the first one.
+ */
+ for (fidp->ref++; fidp; fidp = fidp_next) {
+ if (fidp->path.size == path->size &&
+ !memcmp(fidp->path.data, path->data, path->size)) {
/* Mark the fid non reclaimable. */
fidp->flags |= FID_NON_RECLAIMABLE;
/* reopen the file/dir if already closed */
err = v9fs_reopen_fid(pdu, fidp);
if (err < 0) {
+ put_fid(pdu, fidp);
return err;
}
+ }
+
+ fidp_next = QSIMPLEQ_NEXT(fidp, next);
+
+ if (fidp_next) {
/*
- * Go back to head of fid list because
- * the list could have got updated when
- * switched to the worker thread
+ * Ensure the next fid survives a potential clunk request during
+ * put_fid() below and v9fs_reopen_fid() in the next iteration.
*/
- if (err == 0) {
- goto again;
- }
+ fidp_next->ref++;
}
+
+ /* We're done with this fid */
+ put_fid(pdu, fidp);
}
+
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PULL 5/5] 9pfs: Convert reclaim list to QSLIST
2021-01-25 11:44 [PULL 0/5] 9p next patches Greg Kurz
` (3 preceding siblings ...)
2021-01-25 11:44 ` [PULL 4/5] 9pfs: Improve unreclaim loop Greg Kurz
@ 2021-01-25 11:44 ` Greg Kurz
2021-01-25 15:56 ` [PULL 0/5] 9p next patches Peter Maydell
5 siblings, 0 replies; 7+ messages in thread
From: Greg Kurz @ 2021-01-25 11:44 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Christian Schoenebeck, Greg Kurz
Use QSLIST instead of open-coding for a slightly improved readability.
No behavioral change.
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Message-Id: <20210122143514.215780-1-groug@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/9pfs/9p.c | 17 ++++++++---------
hw/9pfs/9p.h | 2 +-
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 3864d014b43c..5a6e2c9d3d7f 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -416,7 +416,9 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
{
int reclaim_count = 0;
V9fsState *s = pdu->s;
- V9fsFidState *f, *reclaim_list = NULL;
+ V9fsFidState *f;
+ QSLIST_HEAD(, V9fsFidState) reclaim_list =
+ QSLIST_HEAD_INITIALIZER(reclaim_list);
QSIMPLEQ_FOREACH(f, &s->fid_list, next) {
/*
@@ -448,8 +450,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
* a clunk request won't free this fid
*/
f->ref++;
- f->rclm_lst = reclaim_list;
- reclaim_list = f;
+ QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);
f->fs_reclaim.fd = f->fs.fd;
f->fs.fd = -1;
reclaim_count++;
@@ -461,8 +462,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
* a clunk request won't free this fid
*/
f->ref++;
- f->rclm_lst = reclaim_list;
- reclaim_list = f;
+ QSLIST_INSERT_HEAD(&reclaim_list, f, reclaim_next);
f->fs_reclaim.dir.stream = f->fs.dir.stream;
f->fs.dir.stream = NULL;
reclaim_count++;
@@ -476,15 +476,14 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
* Now close the fid in reclaim list. Free them if they
* are already clunked.
*/
- while (reclaim_list) {
- f = reclaim_list;
- reclaim_list = f->rclm_lst;
+ while (!QSLIST_EMPTY(&reclaim_list)) {
+ f = QSLIST_FIRST(&reclaim_list);
+ QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
if (f->fid_type == P9_FID_FILE) {
v9fs_co_close(pdu, &f->fs_reclaim);
} else if (f->fid_type == P9_FID_DIR) {
v9fs_co_closedir(pdu, &f->fs_reclaim);
}
- f->rclm_lst = NULL;
/*
* Now drop the fid reference, free it
* if clunked.
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 85fb6930b0ca..00381591ffa2 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -281,7 +281,7 @@ struct V9fsFidState {
int ref;
bool clunked;
QSIMPLEQ_ENTRY(V9fsFidState) next;
- V9fsFidState *rclm_lst;
+ QSLIST_ENTRY(V9fsFidState) reclaim_next;
};
typedef enum AffixType_t {
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PULL 0/5] 9p next patches
2021-01-25 11:44 [PULL 0/5] 9p next patches Greg Kurz
` (4 preceding siblings ...)
2021-01-25 11:44 ` [PULL 5/5] 9pfs: Convert reclaim list to QSLIST Greg Kurz
@ 2021-01-25 15:56 ` Peter Maydell
5 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2021-01-25 15:56 UTC (permalink / raw)
To: Greg Kurz; +Cc: Christian Schoenebeck, QEMU Developers
On Mon, 25 Jan 2021 at 11:44, Greg Kurz <groug@kaod.org> wrote:
>
> The following changes since commit fef80ea073c4862bc9eaddb6ddb0ed970b8ad7c4:
>
> Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2021-01-20' into staging (2021-01-21 10:44:28 +0000)
>
> are available in the Git repository at:
>
> https://gitlab.com/gkurz/qemu.git tags/9p-next-pull-request
>
> for you to fetch changes up to 81f9766b7a16ccfcfd19e0b4525a5eeba862c851:
>
> 9pfs: Convert reclaim list to QSLIST (2021-01-22 18:26:40 +0100)
>
> ----------------------------------------------------------------
> This fixes a Coverity report and improves the fid reclaim logic.
>
> ----------------------------------------------------------------
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread