Re: [Xen-devel] [PATCH v6 01/12] xen: allow only sizeof(bool) variables for boolean_param()

From: "Jürgen Groß" <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v6 01/12] xen: allow only sizeof(bool) variables for boolean_param()
Date: Mon, 9 Mar 2020 15:06:09 +0100	[thread overview]
Message-ID: <4052bb30-9ec4-18c3-c3a8-addcb9e2afb2@suse.com> (raw)
In-Reply-To: <daec17e3-3964-f189-6974-bdf86689f235@suse.com>

On 09.03.20 14:06, Jan Beulich wrote:
> On 09.03.2020 14:01, Jürgen Groß wrote:
>> On 09.03.20 12:55, Jan Beulich wrote:
>>> On 09.03.2020 12:43, Julien Grall wrote:
>>>> On 26/02/2020 12:46, Juergen Gross wrote:
>>>>> @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[];
>>>>>         __kparam __setup_##_var = \
>>>>>             { .name = __setup_str_##_var, \
>>>>>               .type = OPT_BOOL, \
>>>>> -          .len = sizeof(_var), \
>>>>> +          .len = sizeof(_var) + \
>>>>> +                 BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \
>>>>
>>>>    From my understanding, sizeof(bool) is not necessarily 1 (it can be
>>>> greater). While this is fine to use it in Xen, I think we want it to
>>>> always be one when exposed in the hypfs.
>>>
>>> I don't think so: We want variable of type 'bool' to be updated
>>> consistently (i.e. by a write to the full variable). Hence I
>>> think sizeof(bool) is correct here. I can see though that the
>>> hypercall interface then gains a dependency on the hypervisor's
>>> representation of 'bool', but I think such ought to be taken
>>> care of in the function carrying out the write, not in the
>>> macro here.
>>
>> So you think I should special case bool entries when returning the
>> size information? Or do you think its fine to have the hypervisor's
>> size reported and let the lib do the size handling correctly?
> 
> Either way would be fine by me, but I think not having callers have
> a (required) way to know the hypervisor's sizeof(bool) would be a
> more clean interface overall.

The size is reported via the dirent, so this is fine. And when you have
a look into patch 5 you'll see that the writing of the bool is merged
with the uint writing in the lib code. I should remove the safety check
regarding sizeof(bool) matching the size reported in the dirent, however
(this is a leftover I forgot to remove).

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel