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