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

[Alon Levy]

Hi,

----- "Gerd Hoffmann" <kraxel at redhat.com> wrote:

> Hi,
> 
> > -__visible__ void spice_server_vdi_port_wakeup(SpiceVDIPortInstance
> *sin)
> > +__visible__ void
> spice_server_vdi_port_wakeup(SpiceCharDeviceInstance *sin)
> 
> This should be renamed too.  Also please make a separate patch which 
> does the pure vdiport -> chardevice renaming.
> 

ok.

> > +__visible__ int spice_server_char_device_recognized_subtype(const
> char* subtype)
> > +{
> > +    return (strcmp(subtype, "vdagent") == 0 || strcmp(subtype,
> "smartcard") == 0);
> > +}
> > +
> > +__visible__ const char*
> spice_server_char_device_supported_subtypes(void)
> > +{
> > +    return "vdagent, smartcard";
> > +}
> 
> Hmm?  Shouldn't be one function which returns a list be enougth?
> 
Alex said same thing, will fix.

> > +int spice_server_char_device_add_interface(SpiceServer *s,
> > +                                           SpiceBaseInstance *sin)
> > +{
> > +    SpiceCharDeviceInstance* char_device =
> > +            SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
> > +    SpiceCharDeviceInterface* sif;
> > +
> > +    sif = SPICE_CONTAINEROF(char_device->base.sif,
> SpiceCharDeviceInterface, base);
> > +    const char* subtype = sif->subtype(char_device);
> > +    if (strcmp(subtype, "vdagent") == 0) {
> > +        if (vdagent) {
> > +            red_printf("vdi port already attached");
> > +            return -1;
> > +        }
> > +        char_device->wakeup =&spice_server_vdi_port_wakeup;
> > +        attach_to_red_agent(char_device);
> > +    } else if (strcmp(subtype, "smartcard") == 0) {
> > +        red_printf("smart card not implemented");
> > +        return -1;
> 
> What is planned here?  How will the smartcard data travel from server
> to 
> client (and back)?
> 

There will be a ChannelSmartCard and some internal struct to
keep track of state (reader id's mainly). Messages from
the protocol here http://cgit.freedesktop.org/~alon/cac_card/tree/vscard_common.h
will either be handled directly (add/remove reader) or passed on
to usb-ccid device to handle (anything else).

So it's
client -> smartcard_channel ->|server| handle_message -> smart_card_handle_message
(I'm making this up, but generally the same as we dispatch the other channels)
and right on to a smartcard CharDeviceInstance* and then to write, and on the
other direction wakeup, calls smart_card_wakeup, which calls read etc. and finally
sends messages through the channel using marshaller.
 

> > -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);
> 
> No.
> 

Is this like alex's "replace with const char*" or something else? The comment
will go away.

> > +    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);
> 
> When touching this anyway we should take care to fixup the flow
> control 
> issues this interface has too.  When this is done we should be able to
> 
> move it from spice-experimental.h to spice.h
> 

So please elaborate - you want an equivalent of the state callback but
for the device to call to notify spice?

> >   };
> >
> > -struct SpiceVDIPortInstance {
> > -    SpiceBaseInstance base;
> 
>         const char         *subtype;
> 
> > -    SpiceVDIPortState *st;
> > +struct SpiceCharDeviceInstance {
> > +    SpiceBaseInstance       base;
> > +    SpiceCharDeviceState    *st;
> > +    /* called by spice embedder */
> > +    void (*wakeup)(SpiceCharDeviceInstance *sin);
> >   };
> 
> cheers,
>    Gerd