Re: [Xen-devel] [PATCH RFC 3/4] xen: implement SCHEDOP_soft_reset

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.06.15 at 14:10, <vkuznets@xxxxxxxxxx> wrote:
> "Jan Beulich" <JBeulich@xxxxxxxx> writes:
> 
>>>>> On 22.06.15 at 18:24, <vkuznets@xxxxxxxxxx> wrote:
>>> "Jan Beulich" <JBeulich@xxxxxxxx> writes:
>>> 
>>>>>>> On 22.06.15 at 18:00, <vkuznets@xxxxxxxxxx> wrote:
>>>>> "Jan Beulich" <JBeulich@xxxxxxxx> writes:
>>>>> 
>>>>>>>>> On 03.06.15 at 15:35, <vkuznets@xxxxxxxxxx> wrote:
>>>>>>> @@ -1129,8 +1129,9 @@ void unmap_vcpu_info(struct vcpu *v)
>>>>>>>      mfn = v->vcpu_info_mfn;
>>>>>>>      unmap_domain_page_global((void *)
>>>>>>>                               ((unsigned long)v->vcpu_info & 
>>>>>>> PAGE_MASK));
>>>>>>> -
>>>>>>> -    v->vcpu_info = &dummy_vcpu_info;
>>>>>>> +    v->vcpu_info = ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
>>>>>>> +                    ? (vcpu_info_t *)&shared_info(d, 
>>>>>>> vcpu_info[v->vcpu_id])
>>>>>>
>>>>>> Is this cast really needed?
>>>>>>
>>>>> 
>>>>> Without it my gcc-4.8.3 complains:
>>>>> 
>>>>> domain.c: In function âunmap_vcpu_infoâ:
>>>>> domain.c:1158:21: error: pointer type mismatch in conditional expression 
>>>>> [-Werror]
>>>>>                      : &dummy_vcpu_info);
>>>>>                      ^
>>>>> cc1: all warnings being treated as errors
>>>>
>>>> Which is the kind of warning one normally should _not_ work
>>>> around by adding a cast.
>>> 
>>> In this (and in alloc_vcpu() from where this expression was copied)
>>> particular case this is probably OK: in struct shared_info we have
>>> 'struct vcpu_info vcpu_info[XEN_LEGACY_MAX_VCPUS]' as its first
>>> member. But I may be missing something..
>>
>> Did you read the comment accompanying the definition of
>> __shared_info()?
>>
>> The cast is presumably safe here, but it doesn't _look_ safe. And
>> for future readers (and future changes) it would be better if it did.
> 
> Sorry, it seems I don't undestand the suggestion on how to make this
> look better.

I didn't suggest anything in particular; all I care about is for such
casts to be avoided.

> With CONFIG_COMPAT in both struct shared_info and struct
> compat_shared_info vcpu_info array is of type 'struct vcpu_info[]' but
> v->vcpu_info is of type 'vcpu_info_t *' which means union of 'struct
> vcpu_info' and 'struct compat_vcpu_info' in CONFIG_COMPAT case. Both
> these structures have same size of '64' on x86 so it's really up to its
> user how to treat this data...

Sure, but that doesn't make the cast any more safe or look any
better. But then again I see we already have a similar construct in
alloc_vcpu(), i.e. - okay, keep it as is.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel