Re: [Qemu-devel] [PATCH] Provide a VCARD_DIRECT implementation.

[Jeremy White]

On 01/19/2015 11:16 AM, Marc-André Lureau wrote:
> Hey
> 
> Thanks for proposing passthrough mode. It can be useful in some cases,
> although it's good to mention it's not safe to concurrently share a
> card with other applications since no locking is provided (and I don't
> know if and how it's possible atm).

Actually, I'm beginning to think that this will work cleanly.  pcscd
works to keep clients segregated, and the libnss path, through coolkey,
eventually resolves into pcsc calls as well.  So I don't think this
pathway is fundamentally different than what we're already doing.

With that said, I have found that the spice server does not relay all
smartcard events; there are some events (e.g. a reset) that are short
circuited in the spice server.  That should be fixable without too much
effort, though, I think.

> 
> You should fix your patch to pass qemu scripts/checkpatch.pl (too many
> errors to report here)

*blush*  Yup.

> 
> Some review notes follow.
> 
> On Mon, Jan 19, 2015 at 4:00 PM, Jeremy White <address@hidden> wrote:
>> This enables a passthru mode in the spice client, where we relay
>> apdus from the client to the host, rather than passing through nss
>> and emulating a card.
> 
> Could you explain how you handle multiple readers, and what is the
> sender PID used for?

>From the perspective of this library as a separate entity, which is the
approach I was considering as I designed it, multiple card readers
should work well.  I am not sure if they will flow through the spice
stack and work well; I will need to buy a second reader to test that.
My hunch is that they will not.

As for the PID, that, and the whole queue system, was built because I
had the impression that pcsc-lite was not necessarily thread safe.
(Older versions had a configure check for that option).  It appears as
though that may be an unfounded fear.  I'll experiment with building a
version that eliminates this and see if it works.

> Although I am not very picky about splitting things, I would say you
> could easily split the configure part first.

I'll trust your instincts around the preferences in this project.

>>
>> +# check for pcsclite for smartcard passthru support
> 
> (perhaps it's good to keep in mind that this code should be fairly
> easy to port to winscard, perhaps a TODO would be worth here?)

Sure.

> 
>> +if test "$smartcard_pcsc" != "no"; then
>> +  cat > $TMPC << EOF
>> +#include <winscard.h>
>> +int main(void) { SCardEstablishContext(0, 0, 0, 0); return 0; }
>> +EOF
>> +    # FIXME: do not include $glib_* in here
>> +    pcsc_libs="$($pkg_config --libs libpcsclite 2>/dev/null) $glib_libs"
>> +    pcsc_cflags="$($pkg_config --cflags libpcsclite 2>/dev/null) 
>> $glib_cflags"
> 
> It seems you can remove the $glib here.

Hmm.  I tried that, and failed, in a way that seemed identical to the
check for libnss.  I'll try again.

> 
> Why not use GQueue or GAsyncQueue?

No good reason; I think when I initially wrote it, I hadn't realized
that glib was going to be a hard dependency.  (I'll also clean up the
malloc() call).

>> +static int new_event_thread(PCSCContext *pc)
>> +{
>> +    pc->thread = PR_CreateThread(PR_SYSTEM_THREAD, event_thread,
>> +                     pc, PR_PRIORITY_NORMAL, PR_GLOBAL_THREAD,
>> +                     PR_UNJOINABLE_THREAD, 0);
> 
> I would rather use GThread rather than depending on NSPR here (also
> because you use GMutex and friends).

Sure.  My instinct was to use the same mechanism already in use by this
library, which is the PR_CreateThread, but I think you're right.

>> +#if defined(CONFIG_SMARTCARD_PCSC)
>> +    if (options->use_hw && options->hw_card_type == VCARD_EMUL_PASSTHRU) {
>> +        if (options->vreader_count > 0) {
>> +            fprintf(stderr, "Error: you cannot use a soft card and a 
>> passthru card simultaneously.\n");
>> +            return VCARD_EMUL_FAIL;
>> +        }
>> +
>> +        if (capcsc_init()) {
> 
> Why is this needed? Would it be possible to initialize implicitely
> when using passthrough?

I'm not sure I understand the benefit to that.  In order to detect
reader add/remove, we need to have a monitoring thread.  We need to
start that thread somewhere.

I'll do the fairly obvious work I should have done and resubmit.

Thanks for the review!

Cheers,

Jeremy