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

[Anthony Liguori]

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