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

[Alon Levy]

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<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.
> >>
> >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
>