[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device
From: |
Alon Levy |
Subject: |
Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device |
Date: |
Tue, 25 Jan 2011 18:21:16 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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?
>
> >+#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
>
- [Qemu-devel] [PATCH 4/7] ccid: add ccid-card-emulated device (v2), (continued)
[Qemu-devel] [PATCH 5/7] ccid: add docs, Alon Levy, 2011/01/11
[Qemu-devel] [PATCH 6/6] ccid: configure: add --enable-smartcard and --disable-smartcard, Alon Levy, 2011/01/11
[Qemu-devel] [PATCH 2/7] ccid: add passthru card device, Alon Levy, 2011/01/11
Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device, Alon Levy, 2011/01/28
Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device, Anthony Liguori, 2011/01/28
Re: [Qemu-devel] [PATCH 2/7] ccid: add passthru card device, Alon Levy, 2011/01/30
[Qemu-devel] [PATCH 7/7] ccid: add qdev description strings, Alon Levy, 2011/01/11
[Qemu-devel] [PATCH 6/7] ccid: configure: add --enable/disable and nss only disable, Alon Levy, 2011/01/11
[Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus, Alon Levy, 2011/01/11
[Qemu-devel] [PATCH 3/7] libcacard: initial commit after coding style fixes, Alon Levy, 2011/01/11