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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel