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

[Jeremy White]

Thanks for the careful review.

>> +static void push_apdu(smartcard_ccid_t *ccid, void *data, int len)
>> +{
>> +    apdu_t *a = calloc(1, sizeof(*a) + len);
>> +    apdu_t **p;
>> +
>> +    a->data = malloc(len);
>> +    memcpy(a->data, data, len);
>
> 1. No need to add ( + len ) to the calloc above
>      as a->data is malloced.
>      Is that space is used somewhere else  ?

No, you're right; that's a leftover from a different approach.

> 2. missing a->len = len

*blush*  Deleted some debugging before sending...deleted one line too many.

> 3. Nitpick: I think it's more readable to explicitly add a->next = NULL
>      even though it is 0 from the calloc (maybe use malloc
>      as all fields are set). Not that important.

I don't feel strongly; I'll change it as a form of thanks <grin>.

>> +static void * lun_thread(void *arg)
>> +{
>> +    char buf[8096];
>> +    static int pos = 0;
>
> Why have pos static (especially when buf is not) ?

Artifact of an earlier implementation; good catch.

>> +    for (i = 0; i < MAX_LUNS; i++)
>> +        if (luns[i].fd != -1 && luns[i].lun == Lun) {
>> +            while (p = pop_apdu(&luns[i]))
>> +                free_apdu(p);
>
> Are those left-overs from previous commands ?
> Are those apdu not important enough to be processed ?
>
> Reading below I see that some (all?) of those are "late" apdu's
> coming in after the timeout.

I think you mostly answered this one.

To be fair, this code is solving a theoretical problem, not one I 
encountered in testing.  I think the reasonable, if unlikely, case is an 
instance where an application requests data from a smart card, but then 
either aborts or times out, without clearing the response from the pipeline.

>
>> +
>> +            if (send_tx_buffer(&luns[i], TxBuffer, TxLength)) {
>> +                for (j = 0; j < TX_MAX_SLEEP; j++)
>> +                    if (p = pop_apdu(&luns[i]))
>> +                        break;
>> +                    else
>> +                        usleep(TX_SLEEP_INTERVAL);
>> +
>> +                if (p) {
>> +                    memcpy(RxBuffer, p->data, MIN(p->len, *RxLength));
>> +                    *RxLength = MIN(p->len, *RxLength);
>> +                    free_apdu(p);
>> +                    return IFD_SUCCESS;
>> +                }
> maybe better here: else return IFD_RESPONSE_TIMEOUT

Yes.

>> +            }
>> +        }
>> +    return IFD_ERROR_NOT_SUPPORTED;
> and here IFD_ICC_NOT_PRESENT  (?)

I think actually IFD_NO_SUCH_DEVICE would be more correct.

>> +    unlink(qxl->smartcard_file);
>
> Another nitpick:
> I see the vdagent unix-domain-socket file is not
> unlinked in the code. It is done in scripts/Xspice.

Hmm.  I'm not sure why Alon chose to do it that way.

But the unlink feels right to me; I can't think of a benefit of removing 
it.  And removing it would create a perpetual chore for any caller 
(whether script or otherwise).

Cheers,

Jeremy