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
