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

[Alon Levy]

On Mon, Mar 14, 2011 at 04:20:22PM +0100, Jes Sorensen wrote:

ok, here is a note where I kinda ignored my own wishes but I want
to be very clear on them:
 libcacard should not be part of qemu.
 it is here because I once thought it would speed things up.

So I'm not taking it out or anything - it's fine with me that it
goes into qemu, just as long as it's understood that I'm now maintaining
another copy of it for usage outside of qemu, in the spice client (or
any other client for that matter - it will be the same when we do vnc
support for this).

If I start using qemu-ism then I end up having something I have
to change outside of qemu. Like using QemuMutex.

Another option is for me to check a define, i.e. QEMU_EMBEDDED.

I'm fine with that.

This goes for malloc too of course. And for assuming it never fails. Actually
the later I could change the library to have it's internal malloc that does
an assert (it's ok with the client to abort on oom).

rest of my comments are inline.

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

NSS I assume.

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

will fix.

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

no idea, will check.

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

probably nothing.

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

will man.

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

ok.

> > 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.
> 

will.

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

will fix.

> > +#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.

will split.

> 
> Jes
>