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

[Alon Levy <alevy@redhat.com>]

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