Am 14.03.2019 um 18:27 hat Peter Maydell geschrieben: > On Tue, 12 Mar 2019 at 17:30, Kevin Wolf <kw...@redhat.com> 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.
raw_reconfigure_getfd() returns a file descriptor that works for the given parameters. This can be the existing one (the ret == s->fd case) or a new one. We only own the reference and need to store it if it's a new one. So yes, I think it is a false positive. Kevin