Re: [PATCH] tools/libxc: use uint32_t for pirq in xc_domain_irq_permission

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/07/2021 02:14, Igor Druzhinin wrote:
> On 08/07/2021 02:11, Andrew Cooper wrote:
>> On 08/07/2021 02:08, Igor Druzhinin wrote:
>>> On 07/07/2021 10:19, Andrew Cooper wrote:
>>>> On 07/07/2021 08:46, Jan Beulich wrote:
>>>>>> --- a/tools/include/xenctrl.h
>>>>>> +++ b/tools/include/xenctrl.h
>>>>>> @@ -1385,7 +1385,7 @@ int xc_domain_ioport_permission(xc_interface
>>>>>> *xch,
>>>>>>      int xc_domain_irq_permission(xc_interface *xch,
>>>>>>                                 uint32_t domid,
>>>>>> -                             uint8_t pirq,
>>>>>> +                             uint32_t pirq,
>>>>>>                                 uint8_t allow_access);
>>>>> Take the opportunity and also change "allow_access" to bool? Or is
>>>>> use of bool prohibited in external interfaces?
>>>>
>>>> We've got bool's in the interface already.
>>>
>>> Where exactly? I couldn't find a single "bool".
>>
>> $ git grep -w bool -- :/tools/include/xen*.h
>> ../tools/include/xenctrl.h:1844:                          uint32_t
>> domid, bool restore,
>> ../tools/include/xenctrl.h:1846:                          unsigned int
>> nr_features, bool pae, bool itsc,
>> ../tools/include/xenctrl.h:1847:                          bool
>> nested_virt, const struct xc_xend_cpuid *xend);
>> ../tools/include/xenctrl.h:1954:int
>> xc_altp2m_get_domain_state(xc_interface *handle, uint32_t dom, bool
>> *state);
>> ../tools/include/xenctrl.h:1955:int
>> xc_altp2m_set_domain_state(xc_interface *handle, uint32_t dom, bool
>> state);
>>
>> and loads more.
>
> Are we ok to have different types in ABI interface and in libxc
> function prototype then?

Yes.  Again, we've got plenty of examples like this.

> Because I was referring to ABI structures.

The hypercall structs can't contain bool.  bool has implementation
defined width in C, just like enum, and there is no requirement for
sizeof(bool) to be 1.

The pre-existing uint8_t here is correct, although the hypercall handler
ideally wants a further adjustment to reject non-boolean values.  This
hypercall clearly predates our more careful review practices...

~Andrew