Re: [Qemu-devel] [PATCH 1/1] spice: add chardev

[Alon Levy <alevy@redhat.com>]

On Thu, Dec 16, 2010 at 02:21:16PM +0100, Gerd Hoffmann wrote:
> On 12/16/10 12:29, Alon Levy wrote:
> >Adding a chardev backend for spice, for usage by spice vdagent in
> >conjunction with a properly named virtio-serial device.
> 
> Usage example would be nice here.
> 
> >+#ifdef CONFIG_SPICE
> >+#include "spice-qemu-char.h"
> >+#endif
> 
> #ifdef can be dropped.
> 
ok.

> >+#ifdef CONFIG_SPICE
> >+        {
> >+            .name = "name",
> >+            .type = QEMU_OPT_STRING,
> >+        },{
> >+            .name = "debug",
> >+            .type = QEMU_OPT_NUMBER,
> >+        },
> >+#endif
> 
> This too.

ok.

> 
> >@@ -1381,7 +1384,10 @@ Backend is one of:
> >  @option{stdio},
> >  @option{braille},
> >  @option{tty},
> >-@option{parport}.
> >+@option{parport}
> >+#if defined(CONFIG_SPICE)
> >+@option{spicevmc}.
> >+#endif
> 
> This too, documentation should be there unconditionally.
> 
ok.

> >+//#define SPICE_QEMU_CHAR_USE_IOCTL
> 
> Why is this disabled?
> Does it depend on the chardev patches from Amit?
> 

There was a long discussion that concluded we don't want IOCTL's at all,
and that there should be some other mechanism for connection state
communication between the two sides. Meanwhile I found out I don't need
these (I don't remember exactly what I used instead, but basically just
the regular results of write/read).

> >diff --git a/spice-qemu-char.h b/spice-qemu-char.h
> >new file mode 100644
> >index 0000000..32d5009
> >--- /dev/null
> >+++ b/spice-qemu-char.h
> >@@ -0,0 +1,9 @@
> >+#ifndef __SPICE_QEMU_CHAR_H__
> >+#define __SPICE_QEMU_CHAR_H__
> >+
> >+#include "qemu-char.h"
> >+
> >+CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
> >+
> >+#endif // __SPICE_QEMU_CHAR_H__
> >+
> 
> Hmm, maybe add this to ui/qemu-spice.h instead, so we don't clutter
> the tree with lots of tiny includes?

ok.

> 
> cheers,
>   Gerd