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

[Anthony Liguori <anthony@codemonkey.ws>]

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.

>>> +/* 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