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

[Alon Levy]

On 11/21/2014 05:03 PM, Jeremy White wrote:
> 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).

I agree with both of you for what it's worth.

> 
> Cheers,
> 
> Jeremy
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel