[Spice-devel] [xf86-video-qxl v5] Enable smartcard support for XSpice.

[Jeremy White]

Hi Uri,

>> +AC_ARG_ENABLE([ccid],
>> +            [AS_HELP_STRING([--enable-ccid],
>> +            [Build the spiceccid SmartCard driver (default is no)])],
>> +            [enable_ccid=$enableval],
>> +            [enable_ccid=no])
>> +AC_ARG_WITH(ccid-module-dir,
>> +            [AS_HELP_STRING([--with-ccid-module-dir=DIR ],
>> +            [Specify the install path for spiceccid driver (default
>> is $libdir/pcsc/drivers/serial)])],
>> +            [ cciddir="$withval" ],
>> +            [ cciddir="$libdir/pcsc/drivers/serial" ])
>> +AC_SUBST(cciddir)
>
> ccid requires xspice (rephrase error message at will):
>
> +if test "x$enable_ccid" != "xno" && test "x$enable_xspice" = "xno"; then
> +    AC_MSG_ERROR([Building with ccid requires xspice, but xspice is not
> enabled])
> +fi

Sure.

>> +static void process_reader_add(smartcard_ccid_t *ccid, VSCMsgHeader
>> *h, char *data)
>> +{
>> +    ccid->state |= STATE_READER_ADDED;
>
> ccid->state &= ~STATE_READER_REMOVED;

Yeah, caught that with Marc-Andre's comments.

>> +static void process_card_remove(smartcard_ccid_t *ccid, VSCMsgHeader *h)
>> +{
>> +    ccid->atr_len = 0;
>> +    memset(ccid->atr, 0, sizeof(ccid->atr));
>> +    send_reply(ccid, VSC_SUCCESS);
>
> Trying to understand the flow:
> Is there a way (API) to notify pcsc-lite ?
> Does it get to know about this in the next IFDHTransmitToICC, where
> it tries to send as if nothing happened and receive an error code ?
>
>

I'm not aware of any way to notify pcscd through the IFD protocol of a 
closure.  Afaict, it just uses those error codes as a signal for device 
removal.

As a side note, I have patches for pcsc-lite that I have been using 
which enable user-mode operation for pcscd, and also allow a 'oneshot' 
capability - run once and die.  I've shared those patches with the 
upstream project, but they were against an old version.  I'm hoping to 
respin them for the modern version shortly.


>> +    return(MIN(len, h.length + sizeof(h)));
>
> Nitpick: Note that for sure len >= sizeof(h) + h.length as it's checked
> at the beginning of the function,
> So it should be safe to return h.length + sizeof(h) (as long as that
> check stays in the function).
> Possibly the compiler optimization takes care of this.

Yeah, you're right.


>> +RESPONSECODE IFDHCloseChannel(DWORD Lun)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < MAX_LUNS; i++) {
>> +        if (luns[i].fd != -1 && luns[i].lun == Lun) {
>> +            pthread_cancel(luns[i].tid);
>> +            close(luns[i].fd);
>> +            luns[i].fd = -1;
>> +            luns[i].lun = 0;
>> +            luns[i].atr_len = 0;
>
> luns[i].state &= ~STATE_READER_OPEN;

Sure.

>> +static int smartcard_read(SpiceCharDeviceInstance *sin, uint8_t *buf,
>> int len)
>> +{
>> +    int rc;
>> +
>> +    if (smartcard_sin.fd == -1)
>> +        return 0;
>> +
>
> Maybe retry immediately for EINTR:
>
> do {
>     rc = read()
> } while (! ((rc == -1) && (errno == EINTR)))
>
> and remove "|| errno == EINTR" below.

Can you explain why?  I imagine that the EINTR case should be quite 
rare, and that in those cases, it would return into the regular channel 
handling, where it will be flagged as readable, and eventually return.

Thanks for the further review; it is greatly appreciated.

Cheers,

Jeremy