Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.03.2021 14:25, Andrew Cooper wrote:
> On 12/03/2021 13:08, Jan Beulich wrote:
>> On 12.03.2021 12:32, Andrew Cooper wrote:
>>> On 10/03/2021 10:13, Jan Beulich wrote:
>>>> Sadly I was wrong to suggest dropping vaddrs' initializer during review
>>>> of v2 of the patch introducing this code. gcc 4.3 can't cope.
>>>>
>>>> Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition")
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource(
>>>>      struct grant_table *gt = d->grant_table;
>>>>      unsigned int i, final_frame;
>>>>      mfn_t tmp;
>>>> -    void **vaddrs;
>>>> +    void **vaddrs = NULL;
>>>>      int rc = -EINVAL;
>>>>  
>>>>      if ( !nr_frames )
>>> in v1, there was a companion check.
>>>
>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>> index f937c1d350..2bb07f129f 100644
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource(
>>>      if ( rc )
>>>          goto out;
>>>  
>>> +    /*
>>> +     * Some older toolchains can't spot that vaddrs is non-NULL on
>>> non-error
>>> +     * paths.  Leave some runtime safety.
>>> +     */
>>> +    if ( !vaddrs )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        goto out;
>>> +    }
>>> +
>>>      for ( i = 0; i < nr_frames; ++i )
>>>          mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
>> Oh, I didn't realize this. Will add, but did you really mean to
>> have the function return success in this case (on a release
>> build)? I'd be inclined to put it ahead of if "if ( rc )" and
>> set rc (to e.g. -ENODATA) in this case.
> 
> Oh - quite right.  Returning 0 here will hit the assertion/failsafe
> protecting against livelock.
> 
> I'd be tempted to chose -EINVAL because the only plausible way to get
> here is a bad id, and that path should have errored out earlier.

As you may have seen, I've chosen to stick to ENODATA. This error,
should it ever get raised, would better be easily distinguishable
from an ordinary -EINVAL.

Jan