Re: [PATCH v7 03/19] xen: add and enable Xen console for PVH guests

On Thursday, December 19, 2013 1:54:40 pm Roger Pau Monne wrote:
> diff --git a/sys/dev/xen/console/console.c b/sys/dev/xen/console/console.c
> index 23eaee2..e8079da 100644
> --- a/sys/dev/xen/console/console.c
> +++ b/sys/dev/xen/console/console.c
> _at__at_ -69,11 +69,14 _at__at_ struct mtx              cn_mtx;
>  static char wbuf[WBUF_SIZE];
>  static char rbuf[RBUF_SIZE];
>  static int rc, rp;
> -static unsigned int cnsl_evt_reg;
> +unsigned int cnsl_evt_reg;
>  static unsigned int wc, wp; /* write_cons, write_prod */
>  xen_intr_handle_t xen_intr_handle;
>  device_t xencons_dev;
>  
> +/* Virt address of the shared console page */

Tiny nit: I would expand "Virt" to "Virtual"

> +char *console_page;
> +
>  #ifdef KDB
>  static int	xc_altbrk;
>  #endif
> _at__at_ -110,9 +113,28 _at__at_ static struct ttydevsw xc_ttydevsw = {
>          .tsw_outwakeup	= xcoutwakeup,
>  };
>  
> +/*----------------------------- Debug function -------------------------------*/
> +#define XC_PRINTF_BUFSIZE 1024
> +void
> +xc_printf(const char *fmt, ...)
> +{
> +        __va_list ap;
> +        int retval;
> +        static char buf[XC_PRINTF_BUFSIZE];
> +
> +        va_start(ap, fmt);
> +        retval = vsnprintf(buf, XC_PRINTF_BUFSIZE - 1, fmt, ap);
> +        va_end(ap);
> +        buf[retval] = 0;
> +        HYPERVISOR_console_write(buf, retval);
> +}
> +

vsnprintf() always nul-terminates, so you can simplify this slightly to:

	retval = vsnprintf(buf, sizeof(buf), fmt, ap);

OTOH, you can't actually use 'retval' from vsnprintf as it returns the
number of characters that would have been output if the buffer were
infinitely long, not the number of characters output into the string.
From printf(3):

     These functions return the number of characters printed (not including
     the trailing `\0' used to end output to strings) or a negative value if
     an output error occurs, except for snprintf() and vsnprintf(), which
     return the number of characters that would have been printed if the size
     were unlimited (again, not including the final `\0').

So I think what you actually want is this:

void
xc_printf(const char *fmt, ...)
{
	static char buf[XC_PRINTF_BUFSIZE];
	__va_list ap;

	va_start(ap, fmt);
	vsnprintf(buf, sizeof(buf), fmt, ap);
	va_end(ap);
	HYPERVISOR_console_write(buf, strlen(buf));
}

(And I realize this is an old bug, you were just moving an existing function)

>  
> -static void
> -xc_identify(driver_t *driver, device_t parent)
> -{
> -	device_t child;
> -	child = BUS_ADD_CHILD(parent, 0, driver_name, 0);
> -	device_set_driver(child, driver);
> -	device_set_desc(child, "Xen Console");
> -}
> -

Hmm, how does the device get created without an identify routine?

-- 
John Baldwin