From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=59797 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Phm6Q-0005OM-1d for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:51:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Phm66-0005cI-8q for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:50:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44230) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Phm66-0005bx-0m for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:50:26 -0500 Date: Tue, 25 Jan 2011 18:50:19 +0200 From: Alon Levy Subject: Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device Message-ID: <20110125165018.GH21640@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> <20110125162116.GC21640@playa.tlv.redhat.com> <4D3EF955.1020207@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4D3EF955.1020207@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 10:24:53AM -0600, Anthony Liguori wrote: > On 01/25/2011 10:21 AM, Alon Levy wrote: > >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? > > Anything with the suffix '_t' is reserved by the standard library. > > It's a widely violated rule, but we have run into problems from not > obeying it. I thought qemu coding style said something explicitly about using _t for types that alias basic types - this is actually a change I did to comply.. > > >>>+/* 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? > > Yes, also, what's the encoding (UTF-8)? It's not actually printed anywhere right now, so I'd say unspecififed. I'll document UTF-8. > > Regards, > > Anthony Liguori >