Re: [Xen-devel] [PATCH XTF v2] Implement pv_read

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/07/17 09:36, Felix Schmoll wrote:

2017-07-18 17:04 GMT+02:00 Andrew Cooper <andrew.cooper3@xxxxxxxxxx>:

On 18/07/17 15:53, Felix Schmoll wrote:
> Implement reading from PV console. Making use of polling.
>
> Signed-off-by: Felix Schmoll <eggi.innovations@xxxxxxxxx>
> ---
>  common/console.c      | 22 ++++++++++++++++++++++
>  include/xtf/console.h |  2 ++
>  2 files changed, 24 insertions(+)
>
> diff --git a/common/console.c b/common/console.c
> index 7cb2361..5895ccc 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -7,6 +7,8 @@
>  #include <xtf/libc.h>
>  #include <xtf/traps.h>
>
> +#include <xen/sched.h>

Why is this needed?

 

Apparently it's not; will be removed in the next version of this patch.

> +
>  /*
>   * Output functions, registered if/when available.
>   * Possibilities:
> @@ -45,6 +47,26 @@ static size_t pv_console_write_some(const char *buf, size_t len)
>      return s;
>  }
>
> +extern shared_info_t shared_info;

This extern isn't needed.

> +size_t pv_console_read(char *buf, size_t len)
> +{
> +    size_t s = 0;
> +    uint32_t cons, prod;
> +
> +    while ( !test_and_clear_bit(pv_evtchn, shared_info.evtchn_pending) ||
> +            (pv_ring->in_cons == pv_ring->in_prod) )
> +        hypercall_poll(pv_evtchn);
> +
> +    cons = pv_ring->in_cons, prod = LOAD_ACQUIRE(&pv_ring->in_prod);
> +
> +    while ( (s < len) && (0 < (prod - cons)) )
> +        buf[s++] = pv_ring->in[cons++ & (sizeof(pv_ring->in) - 1)];
> +
> +    STORE_RELEASE(&pv_ring->in_cons, cons);
> +
> +    return s;
> +}
> +
>  /*
>   * Write some data into the pv ring, synchronously waiting for all data to be
>   * consumed.
> diff --git a/include/xtf/console.h b/include/xtf/console.h
> index 2a93c06..9b3f85d 100644
> --- a/include/xtf/console.h
> +++ b/include/xtf/console.h
> @@ -25,6 +25,8 @@ void init_pv_console(xencons_interface_t *ring,
>  void vprintk(const char *fmt, va_list args) __printf(1, 0);
>  void printk(const char *fmt, ...) __printf(1, 2);
>
> +size_t pv_console_read(char *buf, size_t len);

What behaviour do you intend this function to have wrt partial strings?

I'd prefer the function to be as simple as possible. If a specific output is expected that should be handled by the caller. The function name could be adjusted to pv_console_read_some to reflect that better, in symmetry to pv_console_write_some. That one isn't public though, which is why I didn't do it initially.

Alternatively one could adopt a more well-known function such as readline, as was done with printk in regard to printf.

I am not asking you to change the function (although naming it pv_console_read_some() would be an improvement).

There should however be a description (like the other pv_console_*() functions) of what the intended behaviour is.  This function in particular is more complicated than its write counterpart.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel