[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 13/28] file-posix: Prepare permission code for fd
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PULL 13/28] file-posix: Prepare permission code for fd switching |
Date: |
Thu, 14 Mar 2019 17:27:51 +0000 |
On Tue, 12 Mar 2019 at 17:30, Kevin Wolf <address@hidden> wrote:
>
> In order to be able to dynamically reopen the file read-only or
> read-write, depending on the users that are attached, we need to be able
> to switch to a different file descriptor during the permission change.
>
> This interacts with reopen, which also creates a new file descriptor and
> performs permission changes internally. In this case, the permission
> change code must reuse the reopen file descriptor instead of creating a
> third one.
>
> In turn, reopen can drop its code to copy file locks to the new file
> descriptor because that is now done when applying the new permissions.
Hi -- Coverity doesn't like this function (CID 1399712).
I think this may be a false positive, but could you confirm?
> @@ -2696,12 +2695,78 @@ static QemuOptsList raw_create_opts = {
> static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t
> shared,
> Error **errp)
> {
> - return raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
> + BDRVRawState *s = bs->opaque;
> + BDRVRawReopenState *rs = NULL;
> + int open_flags;
> + int ret;
> +
> + if (s->perm_change_fd) {
> + /*
> + * In the context of reopen, this function may be called several
> times
> + * (directly and recursively while change permissions of the parent).
> + * This is even true for children that don't inherit from the
> original
> + * reopen node, so s->reopen_state is not set.
> + *
> + * Ignore all but the first call.
> + */
> + return 0;
> + }
> +
> + if (s->reopen_state) {
> + /* We already have a new file descriptor to set permissions for */
> + assert(s->reopen_state->perm == perm);
> + assert(s->reopen_state->shared_perm == shared);
> + rs = s->reopen_state->opaque;
> + s->perm_change_fd = rs->fd;
> + } else {
> + /* We may need a new fd if auto-read-only switches the mode */
> + ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags,
> + false, errp);
Coverity says that raw_reconfigure_getfd() returns an fd in 'ret' here...
> + if (ret < 0) {
> + return ret;
> + } else if (ret != s->fd) {
> + s->perm_change_fd = ret;
> + }
> + }
> +
> + /* Prepare permissions on old fd to avoid conflicts between old and new,
> + * but keep everything locked that new will need. */
> + ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
...but this call overwrites that fd, so we might never close it.
I think the answer is that either:
* ret == s->fd and we'll close s->fd later
* or we save ret into s->perm_change_fd
and Coverity just isn't clever enough to realise that if
ret == s->fd then we haven't lost the handle.
Is that right? If so I'll mark it as a false-positive in the UI.
thanks
-- PMM
- Re: [Qemu-devel] [PULL 03/28] qapi: move to QOM path for x-block-latency-histogram-set, (continued)
- [Qemu-devel] [PULL 06/28] qemu-iotests: commit to backing file with auto-read-only, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 07/28] block: Avoid useless local_err, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 11/28] file-posix: Store BDRVRawState.reopen_state during reopen, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 10/28] file-posix: Factor out raw_reconfigure_getfd(), Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 09/28] file-posix: Fix bdrv_open_flags() for snapshot=on, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 12/28] file-posix: Lock new fd in raw_reopen_prepare(), Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 15/28] nvme: fix write zeroes offset and count, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 13/28] file-posix: Prepare permission code for fd switching, Kevin Wolf, 2019/03/12
- Re: [Qemu-devel] [PULL 13/28] file-posix: Prepare permission code for fd switching,
Peter Maydell <=
- [Qemu-devel] [PULL 08/28] block: Make permission changes in reopen less wrong, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 14/28] file-posix: Make auto-read-only dynamic, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 17/28] block: Freeze the backing chain for the duration of the commit job, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 16/28] block: Allow freezing BdrvChild links, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 19/28] block: Freeze the backing chain for the duration of the stream job, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 18/28] block: Freeze the backing chain for the duration of the mirror job, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 20/28] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue(), Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 22/28] block: Allow omitting the 'backing' option in certain cases, Kevin Wolf, 2019/03/12
- [Qemu-devel] [PULL 24/28] block: Add a 'mutable_opts' field to BlockDriver, Kevin Wolf, 2019/03/12