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

[Jes Sorensen]

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

> +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!

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.

> +static void emulated_push_type(EmulatedState *card, uint32_t type)
> +{
> +    EmulEvent *event = (EmulEvent *)malloc(sizeof(EmulEvent));

qemu_malloc()

> +    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()

> +    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()

> +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?

Jes