From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34123 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q0DNV-0008Qn-0f for qemu-devel@nongnu.org; Thu, 17 Mar 2011 09:36:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q0DNT-000558-5R for qemu-devel@nongnu.org; Thu, 17 Mar 2011 09:36:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19598) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q0DNS-000553-Rv for qemu-devel@nongnu.org; Thu, 17 Mar 2011 09:36:35 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2HDaXSb009838 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 17 Mar 2011 09:36:33 -0400 Date: Thu, 17 Mar 2011 15:36:07 +0200 From: Alon Levy Subject: Re: [Qemu-devel] [PATCH 4/7] libcacard: initial commit Message-ID: <20110317133607.GV7413@playa.tlv.redhat.com> References: <1298460024-23591-1-git-send-email-alevy@redhat.com> <1298460024-23591-5-git-send-email-alevy@redhat.com> <4D7E3236.8050507@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D7E3236.8050507@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes Sorensen Cc: qemu-devel@nongnu.org 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 > > +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 > > +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 > > +#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 > > +#include > > + > > +/* > > + * 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 >