[Spice-devel] [RFC 1/2] spice, server: introduce SpiceCharDevice

[Alon Levy]

----- "Alexander Larsson" <alexl at redhat.com> wrote:

> On Wed, 2010-08-25 at 06:53 -0400, Alon Levy wrote:
> > index aede4ce..6c02afb 100644
> > --- a/server/spice-experimental.h
> > +++ b/server/spice-experimental.h
> > @@ -1,26 +1,31 @@
> > -/* vdi port interface */
> > +/* char device interfaces */
> >  
> > -#define SPICE_INTERFACE_VDI_PORT "vdi_port"
> > -#define SPICE_INTERFACE_VDI_PORT_MAJOR 1
> > -#define SPICE_INTERFACE_VDI_PORT_MINOR 1
> > -typedef struct SpiceVDIPortInterface SpiceVDIPortInterface;
> > -typedef struct SpiceVDIPortInstance SpiceVDIPortInstance;
> > -typedef struct SpiceVDIPortState SpiceVDIPortState;
> > +#define SPICE_INTERFACE_CHAR_DEVICE "char_device"
> > +#define SPICE_INTERFACE_CHAR_DEVICE_MAJOR 1
> > +#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 1
> > +typedef struct SpiceCharDeviceInterface SpiceCharDeviceInterface;
> > +typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
> > +typedef struct SpiceCharDeviceState SpiceCharDeviceState;
> >  
> > -struct SpiceVDIPortInterface {
> > +struct SpiceCharDeviceInterface {
> >      SpiceBaseInterface base;
> >  
> > -    void (*state)(SpiceVDIPortInstance *sin, int connected);
> > -    int (*write)(SpiceVDIPortInstance *sin, const uint8_t *buf, int
> len);
> > -    int (*read)(SpiceVDIPortInstance *sin, uint8_t *buf, int len);
> > +    /* called by spice */
> > +    const char* (*subtype)(SpiceCharDeviceInstance *sin);
> 
> I don't think we need a callback here. Just make this a const char *.
> 

No, that doesn't work - the subtype is not a feature of the interface, it's a feature
of the specific instance:

typedef struct SpiceVirtualChannel {
    VirtIOSerialPort      port;
    VMChangeStateEntry    *vmstate;
    SpiceCharDeviceInstance  sin;
...
}

and then:

static const char* vmc_subtype(SpiceCharDeviceInstance* sin)
{
    SpiceVirtualChannel *scd = container_of(sin, SpiceVirtualChannel, sin);

    dprintf(scd, 2, "%s\n", __func__);
    return scd->subtype;
}

so if you run qemu -device spicevmc,subtype=vdagent -device spicevmc,subtype=smartcard
the former will respond with "vdagent" and the later with "spicevmc", despite
having a single SpiceCharDeviceInterface.

Of course this seems to avoid the point of having an interface to begin with - unless
you consider that the spicevmc code is inside qemu, and that it's behavior, other
then this vmc_subtype, is the same for any subtype - that's just data we pass on
to spice. We could pass it on in another manner I guess, but it seems natural to pass
it via the parameters given to the device.

> > +    void (*state)(SpiceCharDeviceInstance *sin, int connected);
> > +    int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf,
> int len);
> > +    int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int
> len);
> >  };
> >  
> > -struct SpiceVDIPortInstance {
> > -    SpiceBaseInstance base;
> > -    SpiceVDIPortState *st;
> > +struct SpiceCharDeviceInstance {
> > +    SpiceBaseInstance       base;
> > +    SpiceCharDeviceState    *st;
> > +    /* called by spice embedder */
> > +    void (*wakeup)(SpiceCharDeviceInstance *sin);
> >  };
> >
> > -void spice_server_vdi_port_wakeup(SpiceVDIPortInstance *sin);
> 
> We've generally avoided having ways to call into spice like this.
> Instead have a spice_server_char_device_wakeup() call which
> internally
> could e.g. call via a function pointer in SpiceCharDeviceState.
> 
> > +int spice_server_char_device_recognized_subtype(const char*
> subtype);
> > +const char* spice_server_char_device_supported_subtypes(void);
> 
> I think its better to have a single call that returns a strv, i.e. a
> NULL terminated char *[]. This can be used to handle both of these
> functions and is a nicer api to support.
> 
> -- 
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
>  Alexander Larsson                                            Red Hat,
> Inc 
>        alexl at redhat.com            alexander.larsson at gmail.com 
> He's a shy Republican assassin with a robot buddy named Sparky. She's
> an 
> artistic thirtysomething Hell's Angel descended from a line of
> powerful 
> witches. They fight crime!