Re: [Qemu-devel] [PATCH 1/7] file-posix: Update open_flags in raw_set_perm()

[Max Reitz]

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 <address@hidden>
>> ---
>>  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