Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device

[Alon Levy]

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<stdint.h>
> >+
> >+#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
>