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

[Anthony Liguori <anthony@codemonkey.ws>]

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?

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

Regards,

Anthony Liguori

> +    uint16_t   port;
> +} VSCMsgReconnect;
> +
> +#endif
>