Re: [Qemu-devel] [PATCH 4/7] libcacard: initial commit

[Jes Sorensen <Jes.Sorensen@redhat.com>]

On 02/23/11 12:20, Alon Levy wrote:
> +/* private data for PKI applets */
> +typedef struct CACPKIAppletDataStruct {
> +    unsigned char *cert;
> +    int cert_len;
> +    unsigned char *cert_buffer;
> +    int cert_buffer_len;
> +    unsigned char *sign_buffer;
> +    int sign_buffer_len;
> +    VCardKey *key;
> +} CACPKIAppletData;

Grouping the ints together would allow for better struct padding.

> +/*
> + *  resest the inter call state between applet selects
> + */

I presume it meant to say 'resets' ?

> diff --git a/libcacard/event.c b/libcacard/event.c
> new file mode 100644
> index 0000000..20276a0
> --- /dev/null
> +++ b/libcacard/event.c
> @@ -0,0 +1,112 @@
> +/*
> + *
> + */

This comment is really spot on :)

> diff --git a/libcacard/mutex.h b/libcacard/mutex.h
> new file mode 100644
> index 0000000..cb46aa7
> --- /dev/null
> +++ b/libcacard/mutex.h

UH OH!

> +/*
> + *  This header file provides a way of mapping windows and linux thread calls
> + *  to a set of macros.  Ideally this would be shared by whatever subsystem we
> + *  link with.
> + */
> +
> +#ifndef _H_MUTEX
> +#define _H_MUTEX
> +#ifdef _WIN32
> +#include <windows.h>
> +typedef CRITICAL_SECTION mutex_t;
> +#define MUTEX_INIT(mutex) InitializeCriticalSection(&mutex)
> +#define MUTEX_LOCK(mutex) EnterCriticalSection(&mutex)
> +#define MUTEX_UNLOCK(mutex) LeaveCriticalSection(&mutex)
> +typedef CONDITION_VARIABLE condition_t;
> +#define CONDITION_INIT(cond) InitializeConditionVariable(&cond)
> +#define CONDITION_WAIT(cond, mutex) \
> +            SleepConditionVariableCS(&cond, &mutex, INFINTE)
> +#define CONDITION_NOTIFY(cond) WakeConditionVariable(&cond)
> +typedef uint32_t thread_t;
> +typedef HANDLE thread_status_t;
> +#define THREAD_CREATE(tid, func, arg) \
> +        CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)func, arg, 0, &tid)
> +#define THREAD_SUCCESS(status) ((status) !=  NULL)
> +#else
> +#include <pthread.h>
> +typedef pthread_mutex_t mutex_t;
> +#define MUTEX_INIT(mutex) pthread_mutex_init(&mutex, NULL)
> +#define MUTEX_LOCK(mutex) pthread_mutex_lock(&mutex)
> +#define MUTEX_UNLOCK(mutex) pthread_mutex_unlock(&mutex)
> +typedef pthread_cond_t condition_t;
> +#define CONDITION_INIT(cond) pthread_cond_init(&cond, NULL)
> +#define CONDITION_WAIT(cond, mutex) pthread_cond_wait(&cond, &mutex)
> +#define CONDITION_NOTIFY(cond) pthread_cond_signal(&cond)
> +typedef pthread_t thread_t;
> +typedef int thread_status_t;
> +#define THREAD_CREATE(tid, func, arg) pthread_create(&tid, NULL, func, arg)
> +#define THREAD_SUCCESS(status)  ((status) == 0)
> +#endif

NACK! This is no good, the code needs to be fixed to use QemuMutex from
qemu-thread.h

In addition, a thing like a mutex feature should be in a separate patch,
 not part of the code that uses it. However QEMU already has a mutex set
so this part needs to go.

> +static VCardAppletPrivate *
> +passthru_new_applet_private(VReader *reader)
> +{
> +    VCardAppletPrivate *applet_private = NULL;
> +    LONG rv;
> +
> +    applet_private = (VCardAppletPrivate *)malloc(sizeof(VCardAppletPrivate));

qemu_malloc()

> +    if (applet_private == NULL) {
> +        goto fail;
> +    }

and it never fails.

> +        if (new_reader_list_len != reader_list_len) {
> +            /* update the list */
> +            new_reader_list = (char *)malloc(new_reader_list_len);

qemu_malloc() again.

Please grep through the full patch set and make sure you do not have any
direct calls to malloc() or free().

> +            /* try resetting the pcsc_lite subsystem */
> +            SCardReleaseContext(global_context);
> +            global_context = 0; /* should close it */
> +            printf("***** SCard failure %x\n", rv);

error_report()

> diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
> new file mode 100644
> index 0000000..e4f0b73
> --- /dev/null
> +++ b/libcacard/vcard_emul_nss.c
[snip]
> +struct VReaderEmulStruct {
> +    PK11SlotInfo *slot;
> +    VCardEmulType default_type;
> +    char *type_params;
> +    PRBool present;

What is PRBool and where does it come from?

> +void
> +vcard_emul_reset(VCard *card, VCardPower power)
> +{
> +    PK11SlotInfo *slot;
> +
> +    if (!nss_emul_init) {
> +        return;
> +    }
> +
> +    /* if we reset the card (either power on or power off), we loose our login
> +     * state */
> +    /* TODO: we may also need to send insertion/removal events? */
> +    slot = vcard_emul_card_get_slot(card);
> +    (void)PK11_Logout(slot);

We don't (void) cast calls like that in QEMU.

> +/*
> + *  NSS needs the app to supply a password prompt. In our case the only time
> + *  the password is supplied is as part of the Login APDU. The actual password
> + *  is passed in the pw_arg in that case. In all other cases pw_arg should be
> + *  NULL.
> + */
> +static char *
> +vcard_emul_get_password(PK11SlotInfo *slot, PRBool retries, void *pw_arg)
> +{
> +    /* if it didn't work the first time, don't keep trying */
> +    if (retries) {
> +        return NULL;
> +    }
> +    /* we are looking up a password when we don't have one in hand */
> +    if (pw_arg == NULL) {
> +        return NULL;
> +    }
> +    /* TODO: we really should verify that were are using the right slot */
> +    return PORT_Strdup(pw_arg);

PORT_Strdup???

> +static const char *
> +find_token(const char *str, char token, char token_end)
> +{
> +    /* just do the blind simple thing */
> +    for (; *str; str++) {
> +        if ((*str == token) || (*str == token_end)) {
> +            break;
> +        }
> +    }
> +    return str;
> +}

What is wrong with strpbrk(3) ?

> +static const char *
> +strip(const char *str)
> +{
> +    for (; *str; str++) {
> +        if ((*str != ' ') && (*str != '\n') &&
> +           (*str != '\t') && (*str != '\r')) {
> +            break;

!isspace() ?

> +static const char *
> +find_blank(const char *str)
> +{
> +    for (; *str; str++) {
> +        if ((*str == ' ') || (*str == '\n') ||
> +           (*str == '\t') || (*str == '\r')) {
> +
> +            break;
> +        }
> +    }
> +    return str;
> +}

strpbrk(3) ?

> diff --git a/libcacard/vcardt.h b/libcacard/vcardt.h
> new file mode 100644
> index 0000000..8bca16e
> --- /dev/null
> +++ b/libcacard/vcardt.h
> @@ -0,0 +1,66 @@
> +/*
> + *
> + */
> +#ifndef VCARDT_H
> +#define VCARDT_H 1
> +
> +/*
> + * these should come from some common spice header file
> + */
> +#include <assert.h>
> +#ifndef ASSERT
> +#define ASSERT assert
> +#endif
> +#ifndef MIN
> +#define MIN(x, y) ((x) > (y) ? (y) : (x))
> +#define MAX(x, y) ((x) > (y) ? (x) : (y))
> +#endif

QEMU uses assert(), not ASSERT(). Please fix.

> diff --git a/libcacard/vreader.c b/libcacard/vreader.c
> new file mode 100644
> index 0000000..f4a0c60
> --- /dev/null
> +++ b/libcacard/vreader.c
> @@ -0,0 +1,526 @@
> +/*
> + * emulate the reader
> + */

What is the license of this file?

> +#include "vcard.h"
> +#include "vcard_emul.h"
> +#include "card_7816.h"
> +#include "vreader.h"
> +#include "vevent.h"
> +
> +/*
> + * System includes
> + */
> +#include <stdlib.h>
> +#include <string.h>
> +
> +/*
> + * spice includes
> + */
> +#include "mutex.h"

No no no

> diff --git a/libcacard/vreadert.h b/libcacard/vreadert.h
> new file mode 100644
> index 0000000..51670a3
> --- /dev/null
> +++ b/libcacard/vreadert.h
> @@ -0,0 +1,23 @@
> +/*
> + *
> + */

Spot on!

General comment, this patch is *way* too big. It really should be a
series of patches adding features one after another. The testing for
cards ought to be separate for example.

Jes