From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=49657 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PiiTh-0004WC-JK for qemu-devel@nongnu.org; Fri, 28 Jan 2011 02:10:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PiiTf-0001ao-QH for qemu-devel@nongnu.org; Fri, 28 Jan 2011 02:10:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:2115) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PiiTf-0001aW-Hg for qemu-devel@nongnu.org; Fri, 28 Jan 2011 02:10:39 -0500 Date: Thu, 27 Jan 2011 23:13:24 +0200 From: Alon Levy Subject: Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device Message-ID: <20110127211323.GA27130@playa.redhat.com> References: <1294735359-4009-1-git-send-email-alevy@redhat.com> <1294735359-4009-3-git-send-email-alevy@redhat.com> <4D3EDB7C.3050604@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D3EDB7C.3050604@codemonkey.ws> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org On Tue, Jan 25, 2011 at 08:17:32AM -0600, Anthony Liguori wrote: > On 01/11/2011 02:42 AM, Alon Levy wrote: > >diff --git a/libcacard/vscard_common.h b/libcacard/vscard_common.h > >new file mode 100644 > >index 0000000..9ff1295 > >--- /dev/null > >+++ b/libcacard/vscard_common.h > > This file (and the .c file) need a coding style pass to fixup > comments and the use of _ as a prefix but I want to focus on the > protocol itself. > > First, let's get a written spec into the wiki. I think it's > important that all of our compatibility protocols are documented in > a more formal way such that can be reviewed by a wider audience. > > >@@ -0,0 +1,130 @@ > >+/* Virtual Smart Card protocol definition > >+ * > >+ * This protocol is between a host implementing a group of virtual smart card > >+ * reader, and a client implementing a virtual smart card, or passthrough to > >+ * a real card. > >+ * > >+ * The current implementation passes the raw APDU's from 7816 and additionally > >+ * contains messages to setup and teardown readers, handle insertion and > >+ * removal of cards, negotiate the protocol and provide for error responses. > >+ * > >+ * Copyright (c) 2010 Red Hat. > >+ * > >+ * This code is licensed under the LGPL. > >+ */ > >+ > >+#ifndef _VSCARD_COMMON_H > >+#define _VSCARD_COMMON_H > >+ > >+#include > >+ > >+#define VERSION_MAJOR_BITS 11 > >+#define VERSION_MIDDLE_BITS 11 > >+#define VERSION_MINOR_BITS 10 > > Distros make versioning not enough. Inevitably, someone wants to > back port a bug fix or a feature for some RHEL7.2 release or > something like that. > > Feature negotiation has worked pretty well for us and I'd suggest > using it within the protocol. > > >+#define MAKE_VERSION(major, middle, minor) \ > >+ ( (major<< (VERSION_MINOR_BITS + VERSION_MIDDLE_BITS)) \ > >+ | (middle<< VERSION_MINOR_BITS) \ > >+ | (minor) ) > >+ > >+/** IMPORTANT NOTE on VERSION > >+ * > >+ * The version below MUST be changed whenever a change in this file is made. > >+ * > >+ * The last digit, the minor, is for bug fix changes only. > >+ * > >+ * The middle digit is for backward / forward compatible changes, updates > >+ * to the existing messages, addition of fields. > >+ * > >+ * The major digit is for a breaking change of protocol, presumably > >+ * something that cannot be accomodated with the existing protocol. > >+ */ > >+ > >+#define VSCARD_VERSION MAKE_VERSION(0,0,1) > >+ > >+typedef enum { > >+ VSC_Init, > >+ VSC_Error, > >+ VSC_ReaderAdd, > >+ VSC_ReaderAddResponse, > >+ VSC_ReaderRemove, > >+ VSC_ATR, > >+ VSC_CardRemove, > >+ VSC_APDU, > >+ VSC_Reconnect > >+} VSCMsgType; > > Should number the enum to be specific at least. > > >+ > >+typedef enum { > >+ VSC_GENERAL_ERROR=1, > >+ VSC_CANNOT_ADD_MORE_READERS, > >+} VSCErrorCode; > >+ > >+typedef uint32_t reader_id_t; > > This namespace is reserved by C. > > >+#define VSCARD_UNDEFINED_READER_ID 0xffffffff > >+#define VSCARD_MINIMAL_READER_ID 0 > >+ > >+typedef struct VSCMsgHeader { > >+ VSCMsgType type; > >+ reader_id_t reader_id; > >+ uint32_t length; > > Is length just the data length or the whole message length? > The data length. Is this enough to document? > >+ uint8_t data[0]; > >+} VSCMsgHeader; > >+ > >+/* VSCMsgInit Client<-> Host > >+ * Host replies with allocated reader id in ReaderAddResponse > >+ * */ > >+typedef struct VSCMsgInit { > >+ uint32_t version; > >+} VSCMsgInit; > >+ > >+/* VSCMsgError Client<-> Host > >+ * */ > >+typedef struct VSCMsgError { > >+ uint32_t code; > >+} VSCMsgError; > >+ > >+/* VSCMsgReaderAdd Client -> Host > >+ * Host replies with allocated reader id in ReaderAddResponse > >+ * name - name of the reader on client side. > >+ * */ > >+typedef struct VSCMsgReaderAdd { > >+ uint8_t name[0]; > > Is this a string? > > >+} VSCMsgReaderAdd; > >+ > >+/* VSCMsgReaderAddResponse Host -> Client > >+ * Reply to ReaderAdd > >+ * */ > >+typedef struct VSCMsgReaderAddResponse { > >+} VSCMsgReaderAddResponse; > >+ > >+/* VSCMsgReaderRemove Client -> Host > >+ * */ > >+typedef struct VSCMsgReaderRemove { > >+} VSCMsgReaderRemove; > >+ > >+/* VSCMsgATR Client -> Host > >+ * Answer to reset. Sent for card insertion or card reset. > >+ * */ > >+typedef struct VSCMsgATR { > >+ uint8_t atr[0]; > >+} VSCMsgATR; > >+ > >+/* VSCMsgCardRemove Client -> Host > >+ * */ > >+typedef struct VSCMsgCardRemove { > >+} VSCMsgCardRemove; > >+ > >+/* VSCMsgAPDU Client<-> Host > >+ * */ > >+typedef struct VSCMsgAPDU { > >+ uint8_t data[0]; > >+} VSCMsgAPDU; > >+ > >+/* VSCMsgReconnect Host -> Client > >+ * */ > >+typedef struct VSCMsgReconnect { > >+ uint32_t ip; > > This is not ipv6 friendly. Two strings would be a better choice. A string for host makes sense, why for port? isn't a 32 bit port enough? > > Regards, > > Anthony Liguori > > >+ uint16_t port; > >+} VSCMsgReconnect; > >+ > >+#endif >