From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39919 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PhlhU-0007XL-PV for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:25:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PhlhT-00040X-17 for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:25:00 -0500 Received: from mail-qy0-f180.google.com ([209.85.216.180]:53223) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PhlhS-00040Q-UF for qemu-devel@nongnu.org; Tue, 25 Jan 2011 11:24:58 -0500 Received: by qyk29 with SMTP id 29so6367543qyk.4 for ; Tue, 25 Jan 2011 08:24:58 -0800 (PST) Message-ID: <4D3EF955.1020207@codemonkey.ws> Date: Tue, 25 Jan 2011 10:24:53 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device 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> In-Reply-To: <20110125162116.GC21640@playa.tlv.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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. >>> +/* 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)? Regards, Anthony Liguori