From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44535 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Phle4-0005UE-7s for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:21:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Phle0-00037T-5q for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:21:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13068) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Phldz-000370-SC for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:21:24 -0500 Date: Tue, 25 Jan 2011 18:21:16 +0200 From: Alon Levy Subject: Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device Message-ID: <20110125162116.GC21640@playa.tlv.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. ok, I'll create Features/Smartcard/Protocol > > >@@ -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. > Suggestion accepted. > >+#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. will fix. > > >+ > >+typedef enum { > >+ VSC_GENERAL_ERROR=1, > >+ VSC_CANNOT_ADD_MORE_READERS, > >+} VSCErrorCode; > >+ > >+typedef uint32_t reader_id_t; > > This namespace is reserved by C. reader_id_t is reserved? > > >+#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? > data length, I'll add a comment. > >+ 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? > Yes. You expect char? > >+} 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. > Will fix. > Regards, > > Anthony Liguori > > >+ uint16_t port; > >+} VSCMsgReconnect; > >+ > >+#endif >