Re: [Qemu-devel] [PATCH v3] xen_console: support the new extended xenstore protocol

[Stefano Stabellini]

On Thu, 30 Jun 2011, Alexander Graf wrote:
> On 06/29/2011 01:12 PM, address@hidden wrote:
> > From: Stefano Stabellini<address@hidden>
> >
> > Since CS 21994 on xen-unstable.hg and CS
> > 466608f3a32e1f9808acdf832a5843af37e5fcec on qemu-xen-unstable.git, few
> > changes have been introduced to the PV console xenstore protocol, as
> > described by the document docs/misc/console.txt under xen-unstable.hg.
> >
> >  From the Qemu point of view, very few modifications are needed to
> > correctly support the protocol: read from xenstore the "output" node
> > that tell us what the output of the PV console is going to be.
> > In case the output is a tty, write to xenstore the device name.
> >
> > Changes in v2:
> >
> > - fix error paths: free malloc'ed strings and close the xenstore
> > connection before returning;
> >
> > - remove useless snprintf in xenstore_store_pv_console_info if i == 0.
> >
> > Changes in v3:
> >
> > - replace xs_daemon_open/xs_daemon_close with xs_open/xs_close.
> >
> > Signed-off-by: Stefano Stabellini<address@hidden>
> > ---
> >   hw/xen.h         |    1 +
> >   hw/xen_console.c |   16 +++++++++-----
> >   xen-all.c        |   60 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 71 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/xen.h b/hw/xen.h
> > index d435ca0..dad0ca0 100644
> > --- a/hw/xen.h
> > +++ b/hw/xen.h
> > @@ -50,6 +50,7 @@ qemu_irq *xen_interrupt_controller_init(void);
> >   int xen_init(void);
> >   int xen_hvm_init(void);
> >   void xen_vcpu_init(void);
> > +void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
> >
> >   #if defined(NEED_CPU_H)&&  !defined(CONFIG_USER_ONLY)
> >   void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size);
> > diff --git a/hw/xen_console.c b/hw/xen_console.c
> > index 519d5f5..e81afcd 100644
> > --- a/hw/xen_console.c
> > +++ b/hw/xen_console.c
> > @@ -179,8 +179,9 @@ static void xencons_send(struct XenConsole *con)
> >   static int con_init(struct XenDevice *xendev)
> >   {
> >       struct XenConsole *con = container_of(xendev, struct XenConsole, 
> > xendev);
> > -    char *type, *dom;
> > +    char *type, *dom, label[32];
> >       int ret = 0;
> > +    const char *output;
> >
> >       /* setup */
> >       dom = xs_get_domain_path(xenstore, con->xendev.dom);
> > @@ -194,11 +195,14 @@ static int con_init(struct XenDevice *xendev)
> >           goto out;
> >       }
> >
> > -    if (!serial_hds[con->xendev.dev])
> > -   xen_be_printf(xendev, 1, "WARNING: serial line %d not configured\n",
> > -                      con->xendev.dev);
> > -    else
> > -        con->chr = serial_hds[con->xendev.dev];
> > +    output = xenstore_read_str(con->console, "output");
> > +    /* output is a pty by default */
> > +    if (output == NULL) {
> > +        output = "pty";
> 
> Not sure I understand. Why would a pty be the default? stdio makes a lot 
> more sense, no?

ptys are always been the default in the xen world. Besides qemu is
always executed by the toolstack, so stdio might be used for something
else...


> > +    }
> > +    snprintf(label, sizeof(label), "xencons%d", con->xendev.dev);
> > +    con->chr = qemu_chr_open(label, output, NULL);
> > +    xenstore_store_pv_console_info(con->xendev.dev, con->chr);
> >
> >   out:
> >       qemu_free(type);
> > diff --git a/xen-all.c b/xen-all.c
> > index 6099bff..3fd04ef 100644
> > --- a/xen-all.c
> > +++ b/xen-all.c
> > @@ -737,6 +737,66 @@ static void cpu_handle_ioreq(void *opaque)
> >       }
> >   }
> >
> > +static int store_dev_info(int domid, CharDriverState *cs, const char 
> > *string)
> > +{
> > +    struct xs_handle *xs = NULL;
> > +    char *path = NULL;
> > +    char *newpath = NULL;
> > +    char *pts = NULL;
> > +    int ret = -1;
> > +
> > +    /* Only continue if we're talking to a pty. */
> > +    if (strncmp(cs->filename, "pty:", 4)) {
> 
> What's wrong with !pty?

Nothing wrong, we don't need to write anything back to xenstore in that
case.