On 08.05.19 15:06, Kevin Wolf wrote:
> Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
>> raw_check_perm() + raw_set_perm() can change the flags associated with
>> the current FD. If so, we have to update BDRVRawState.open_flags
>> accordingly. Otherwise, we may keep reopening the FD even though the
>> current one already has the correct flags.
>>
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> ---
>> block/file-posix.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 1cf4ee49eb..66b46ec0eb 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -145,6 +145,7 @@ typedef struct BDRVRawState {
>> uint64_t locked_shared_perm;
>>
>> int perm_change_fd;
>> + int perm_change_flags;
>> BDRVReopenState *reopen_state;
>>
>> #ifdef CONFIG_XFS
>> @@ -2762,6 +2763,7 @@ static int raw_check_perm(BlockDriverState *bs,
>> uint64_t perm, uint64_t shared,
>
> Adding some context before this hunk:
>
> 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, perm,
> false, errp);
> if (ret < 0) {
>> return ret;
>> } else if (ret != s->fd) {
>> s->perm_change_fd = ret;
>> + s->perm_change_flags = open_flags;
>> }
>> }
>
> s->perm_change_flags is set in the else branch. According to the comment
> in raw_set_perm(), not setting it in the then branch is actually correct
> because .bdrv_reopen_commit will run first, so s->perm_change_flags
> isn't accessed, but wouldn't it be nicer to have a valid value in it
> anyway? Who knows where we'll add accesses later.Why not, it can’t hurt. Max
signature.asc
Description: OpenPGP digital signature
