Re: [Xen-devel] [PATCH v5 3/8] xen: add basic hypervisor filesystem support

From: Julien Grall <julien@xen.org>
To: Juergen Gross <jgross@suse.com>, xen-devel@lists.xenproject.org
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Daniel De Graaf" <dgdegra@tycho.nsa.gov>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v5 3/8] xen: add basic hypervisor filesystem support
Date: Wed, 19 Feb 2020 16:47:26 +0000	[thread overview]
Message-ID: <f614ddb8-6e49-d302-f20c-2011abd9646d@xen.org> (raw)
In-Reply-To: <ac088c1e-0326-b72a-e1bf-582493e4e712@xen.org>

On 19/02/2020 16:26, Julien Grall wrote:
> 
> 
> On 19/02/2020 08:11, Juergen Gross wrote:
>> +int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
>> +                     XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned 
>> long ulen)
>> +{
>> +    union {
>> +        char buf[8];
>> +        uint8_t u8;
>> +        uint16_t u16;
>> +        uint32_t u32;
>> +        uint64_t u64;
>> +    } u;
>> +
>> +    ASSERT(leaf->e.type == XEN_HYPFS_TYPE_UINT && leaf->e.size <= 8);
> 
> Shouldn't this be XEN_HYPFSW_TYPE_BOOL? But then, why are we allowing a 
> boolean parameter to be held in a 64-bit value?
> 
> IHMO, a boolean should really be hold in a bool and nothing else.

After sending my e-mail, I realized the size of the bool is 
implementation defined. However, I think my point stands, it would be 
better to have a fixed size for bool (i.e 1) and deny all the other use.

Cheers,

-- 
Julien Grall

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