[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock f
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor |
Date: |
Thu, 22 Mar 2018 18:39:25 +0100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
[ Cc: qemu-block ]
Am 22.03.2018 um 18:20 hat Dion Bosschieter geschrieben:
> In commit 244a5668106297378391b768e7288eb157616f64 another
> file descriptor to BDRVRawState is added. When we try to issue the
> reopen command only s->fd is reopened; lock_fd could still hold an old
> file descriptor "possibly" pointing to another file.
>
> - change raw_reopen_prepare so it checks use_lock from BDRVRawState and
> tries to reopen lock_fd accordingly
> - change raw_reopen_commit so it closes the old lock_fd on use_lock
>
> Signed-off-by: Dion Bosschieter <address@hidden>
bdrv_reopen() is not meant for opening a different file, it is meant to
change the flags and options of the same file. Do you have a use case
where you would actually need to switch to a different file?
As far as I know, lock_fd was specifically introduced _because_ it stays
the same across reopen, so we don't need a racy release/reacquire pair.
Fam (CCed) should know more.
In any case, doesn't your patch drop all the locks without reacquiring
them on the new lock_fd?
Kevin
> block/file-posix.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index d7fb772c14..16d83fc49e 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -167,6 +167,7 @@ typedef struct BDRVRawState {
>
> typedef struct BDRVRawReopenState {
> int fd;
> + int lock_fd;
> int open_flags;
> } BDRVRawReopenState;
>
> @@ -795,6 +796,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> raw_parse_flags(state->flags, &rs->open_flags);
>
> rs->fd = -1;
> + rs->lock_fd = -1;
>
> int fcntl_flags = O_APPEND | O_NONBLOCK;
> #ifdef O_NOATIME
> @@ -820,6 +822,17 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> rs->fd = -1;
> }
> }
> +
> + if (s->use_lock) {
> + rs->lock_fd = qemu_dup(s->lock_fd);
> + if (rs->lock_fd >= 0) {
> + ret = fcntl_setfl(rs->lock_fd, rs->open_flags);
> + if (ret) {
> + qemu_close(rs->lock_fd);
> + rs->lock_fd = -1;
> + }
> + }
> + }
> }
>
> /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
> @@ -835,6 +848,14 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> error_setg_errno(errp, errno, "Could not reopen file");
> ret = -1;
> }
> +
> + if (s->use_lock) {
> + rs->lock_fd = qemu_open(normalized_filename, rs->open_flags);
> + if (rs->lock_fd == -1) {
> + error_setg_errno(errp, errno, "Could not reopen file for
> locking");
> + ret = -1;
> + }
> + }
> }
> }
>
> @@ -861,7 +882,11 @@ static void raw_reopen_commit(BDRVReopenState *state)
> s->open_flags = rs->open_flags;
>
> qemu_close(s->fd);
> + if (s->use_lock) {
> + qemu_close(s->lock_fd);
> + }
> s->fd = rs->fd;
> + s->lock_fd = rs->lock_fd;
>
> g_free(state->opaque);
> state->opaque = NULL;
> --
> 2.14.2
>