On Mon, Mar 14, 2011 at 04:20:22PM +0100, Jes Sorensen wrote:
> 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.
>
I've got everything done except the split. Is there any value in sending
it without doing the split? could you give me some hints. As I mentioned
off line, this code was not written by me, which makes this harder (but
not impossible)
Alon
> Jes
>
