Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io()

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	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>, Tim Deegan <tim@xen.org>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io()
Date: Mon, 5 Aug 2019 11:17:39 +0100	[thread overview]
Message-ID: <cebbe948-924b-3d63-5be5-c5b693a882a8@arm.com> (raw)
In-Reply-To: <6c0b17c8-2c50-ed56-702f-165bd692699b@suse.com>

Hi Jan,

On 05/08/2019 11:07, Jan Beulich wrote:
> On 05.08.2019 11:40, Julien Grall wrote:
>> Hi,
>>
>> On 09/04/2019 12:42, Jan Beulich wrote:
>>>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:
>>>> On 03/04/2019 14:04, Jan Beulich wrote:
>>>>> Also please note the quotation used by the mentioned
>>>>> existing doc comments, as well as a few other formal aspects
>>>>> (like them also making clear what the return type is). I think
>>>>> that's a model used elsewhere as well, so I think you should
>>>>> follow it here.
>>>>
>>>> I haven't replicated the ` because I have no idea what they are used for. I
>>>> would appreciate if you provide pointer how to use them.
>>>
>>> Well, I can only point you at the history of things, e.g.
>>> 262e118a37 "docs/html/: Annotations for two hypercalls".
>>>
>>>>> The other thing is: As mentioned elsewhere, I don't think the
>>>>> first two parameters should be plain int. I'm not happy to see
>>>>> this proliferate into documentation as well, i.e. I'd prefer if
>>>>> this was corrected before adding documentation. Would you
>>>>> be willing to do this, or should I add it to my todo list?
>>>>
>>>> While switching from cmd from signed to unsigned should be ok. This would
>>>> introduce a different behavior of for count.
>>>
>>> Since this removes an error condition, I think this is an okay change
>>> to happen, without ...
>>>
>>>> Are we happy with that, or shall we add a check ((int)count) > 0?
>>>
>>> ... any such extra check. This also isn't going to introduce any new
>>> real risk of a long running operation or some such - if 2Gb of input
>>> data are fine, I can't see why 4Gb wouldn't be, too.
>>
>> Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB.
> 
> Hmm, valid point.
> 
>> So I would rather limit the buffer to 2GB for everyone.
> 
> Probably fair enough then (if explicitly stated). Personally I would
> nevertheless allow sizes up to 4Gb-4kb, but I can see why this may
> not be liked by everyone. I'd like to point out though that the
> PTR_ERR() machinery we've inherited from Linux utilizes exactly that.

Well, that's a console buffer. The chance you have write/read more than 2GB in a 
row is very unlikely.

So it feels to me that exposing PTR_ERR(...) like interface is not worth it.

Cheers,

-- 
Julien Grall

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