Re: [Qemu-devel] [PATCH 5/7] ccid: add ccid-card-emulated device

[Alon Levy]

On Thu, Mar 17, 2011 at 12:54:16PM +0200, Alon Levy wrote:

sorry for the double review of the review, nothing new here.

> On Mon, Mar 14, 2011 at 04:41:02PM +0100, Jes Sorensen wrote:
> > On 02/23/11 12:20, Alon Levy wrote:
> > > diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
> > > new file mode 100644
> > > index 0000000..bd84d45
> > > --- /dev/null
> > > +++ b/hw/ccid-card-emulated.c
> > > @@ -0,0 +1,599 @@
> > > +/*
> > > + * CCID Card Device. Emulated card.
> > > + *
> > > + * Copyright (c) 2011 Red Hat.
> > > + * Written by Alon Levy.
> > > + *
> > > + * This code is licenced under the GNU LGPL, version 2 or later.
> > > + */
> > > +
> > > +/*
> > > + * It can be used to provide access to the local hardware in a non 
> > > exclusive
> > > + * way, or it can use certificates. It requires the usb-ccid bus.
> > > + *
> > > + * Usage 1: standard, mirror hardware reader+card:
> > > + * qemu .. -usb -device usb-ccid -device ccid-card-emulated
> > > + *
> > > + * Usage 2: use certificates, no hardware required
> > > + * one time: create the certificates:
> > > + *  for i in 1 2 3; do
> > > + *      certutil -d /etc/pki/nssdb -x -t "CT,CT,CT" -S -s "CN=user$i" -n 
> > > user$i
> > > + *  done
> > > + * qemu .. -usb -device usb-ccid \
> > > + *  -device ccid-card-emulated,cert1=user1,cert2=user2,cert3=user3
> > > + *
> > > + * If you use a non default db for the certificates you can specify it 
> > > using
> > > + * the db parameter.
> > > + */
> > > +
> > > +#include <pthread.h>
> > 
> > qemu-thread.h
> ok, fixing.
> 
> > 
> > > +static const char *emul_event_to_string(uint32_t emul_event)
> > > +{
> > > +    switch (emul_event) {
> > > +    case EMUL_READER_INSERT: return "EMUL_READER_INSERT";
> > > +    case EMUL_READER_REMOVE: return "EMUL_READER_REMOVE";
> > > +    case EMUL_CARD_INSERT: return "EMUL_CARD_INSERT";
> > > +    case EMUL_CARD_REMOVE: return "EMUL_CARD_REMOVE";
> > > +    case EMUL_GUEST_APDU: return "EMUL_GUEST_APDU";
> > > +    case EMUL_RESPONSE_APDU: return "EMUL_RESPONSE_APDU";
> > > +    case EMUL_ERROR: return "EMUL_ERROR";
> > 
> > YUCK!
> can we turn down the caps / disgust statements? I understand this
> is a personal affront to you somehow? can we settle this at dawn
> tommorrow?
> 
> > 
> > No multi statements on a single line!
> > 
> > > +#define MAX_ATR_SIZE 40
> > > +struct EmulatedState {
> > > +    CCIDCardState base;
> > > +    uint8_t  debug;
> > > +    char    *backend_str;
> > > +    uint32_t backend;
> > > +    char    *cert1;
> > > +    char    *cert2;
> > > +    char    *cert3;
> > > +    char    *db;
> > > +    uint8_t  atr[MAX_ATR_SIZE];
> > > +    uint8_t  atr_length;
> > > +    QSIMPLEQ_HEAD(event_list, EmulEvent) event_list;
> > > +    pthread_mutex_t event_list_mutex;
> > > +    VReader *reader;
> > > +    QSIMPLEQ_HEAD(guest_apdu_list, EmulEvent) guest_apdu_list;
> > > +    pthread_mutex_t vreader_mutex; /* and guest_apdu_list mutex */
> > > +    pthread_mutex_t handle_apdu_mutex;
> > > +    pthread_cond_t handle_apdu_cond;
> > > +    int      pipe[2];
> > > +    int      quit_apdu_thread;
> > > +    pthread_mutex_t apdu_thread_quit_mutex;
> > > +    pthread_cond_t apdu_thread_quit_cond;
> > > +};
> > 
> > Bad struct packing and wrong thread types.
> will use qemu-thread. that's what you mean by wrong thread types, right?
> s/pthread_thread_t/QemuThread/ etc. (Cond, Mutex)
> 
> > 
> > > +static void emulated_push_type(EmulatedState *card, uint32_t type)
> > > +{
> > > +    EmulEvent *event = (EmulEvent *)malloc(sizeof(EmulEvent));
> > 
> > qemu_malloc()
> yep, fixing.
> 
> > 
> > > +    assert(event);
> > > +    event->p.gen.type = type;
> > > +    emulated_push_event(card, event);
> > > +}
> > > +
> > > +static void emulated_push_error(EmulatedState *card, uint64_t code)
> > > +{
> > > +    EmulEvent *event = (EmulEvent *)malloc(sizeof(EmulEvent));
> > 
> > qemu_malloc()
> fixing.
> 
> > 
> > > +    assert(event);
> > > +    event->p.error.type = EMUL_ERROR;
> > > +    event->p.error.code = code;
> > > +    emulated_push_event(card, event);
> > > +}
> > > +
> > > +static void emulated_push_data_type(EmulatedState *card, uint32_t type,
> > > +    const uint8_t *data, uint32_t len)
> > > +{
> > > +    EmulEvent *event = (EmulEvent *)malloc(sizeof(EmulEvent) + len);
> > 
> > qemu_malloc()
> fixing.
> 
> > 
> > > +static void pipe_read(void *opaque)
> > > +{
> > > +    EmulatedState *card = opaque;
> > > +    EmulEvent *event, *next;
> > > +    char dummy;
> > > +    int len;
> > > +
> > > +    do {
> > > +        len = read(card->pipe[0], &dummy, sizeof(dummy));
> > > +    } while (len == sizeof(dummy));
> > 
> > Shouldn't you check error codes here?
> yes, my bad.
> 
> > 
> > Jes
>