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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 03/04/2019 14:04, Jan Beulich wrote:
> > > > On 02.04.19 at 18:42, <julien.grall@xxxxxxx> wrote:
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
> >   #define __HYPERVISOR_set_timer_op         15
> >   #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */
> >   #define __HYPERVISOR_xen_version          17
> > +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); */
> >   #define __HYPERVISOR_console_io           18
> I pretty strongly object to this comment going where you put it.
> I'd be fine if you put it ...
>
> > @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> >   /* ` } */
> >   
> >   /*
> > - * Commands to HYPERVISOR_console_io().
> > + * Commands to HYPERVISOR_console_io()
> ... into here, just like done for HYPERVISOR_mmuext_op() and
> HYPERVISOR_update_va_mapping*() immediately above. This
> would then also help ...
>
> > + * @cmd: Command (see below)
> > + * @count: Size of the buffer to read/write
> > + * @buffer: Pointer in the guest memory
> ... associating these names.
Ok.

> 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.
> 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.
Are we happy with that, or shall we add a check ((int)count) > 0?

In any case, I am happy to have a look at it.

Cheers,

--
Julien Grall

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