From: Jes Sorensen <Jes.Sorensen@redhat.com>
To: Alon Levy <alevy@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus
Date: Mon, 14 Mar 2011 14:54:59 +0100 [thread overview]
Message-ID: <4D7E1E33.1070801@redhat.com> (raw)
In-Reply-To: <1298460024-23591-2-git-send-email-alevy@redhat.com>
On 02/23/11 12:20, Alon Levy wrote:
> diff --git a/configure b/configure
> index 791b71d..147aab3 100755
> --- a/configure
> +++ b/configure
> @@ -174,6 +174,7 @@ trace_backend="nop"
> trace_file="trace"
> spice=""
> rbd=""
> +smartcard="yes"
IMHO smartcard support shouldn't be enabled per default. The userbase is
limited.
> diff --git a/hw/ccid.h b/hw/ccid.h
> new file mode 100644
> index 0000000..4350bc2
> --- /dev/null
> +++ b/hw/ccid.h
> @@ -0,0 +1,54 @@
> +/*
> + * CCID Passthru Card Device emulation
> + *
> + * Copyright (c) 2011 Red Hat.
> + * Written by Alon Levy.
> + *
> + * This code is licenced under the GNU LGPL, version 2 or later.
> + */
> +
[snip]
> +
> +/* callbacks to be used by the CCID device (hw/usb-ccid.c) to call
> + * into the smartcard device (hw/ccid-card-*.c)
> + */
This is inconsistent with the comment above. Normally multi-line
comments in QEMU are like this:
/*
* foo
* bar
*/
> +struct CCIDCardInfo {
> + DeviceInfo qdev;
> + void (*print)(Monitor *mon, CCIDCardState *card, int indent);
> + const uint8_t *(*get_atr)(CCIDCardState *card, uint32_t *len);
> + void (*apdu_from_guest)(CCIDCardState *card,
> + const uint8_t *apdu,
> + uint32_t len);
> + int (*exitfn)(CCIDCardState *card);
> + int (*initfn)(CCIDCardState *card);
> +};
> +
> +/* API for smartcard calling the CCID device (used by hw/ccid-card-*.c)
> + */
again here
> +void ccid_card_send_apdu_to_guest(CCIDCardState *card,
> + uint8_t *apdu,
> + uint32_t len);
> +void ccid_card_card_removed(CCIDCardState *card);
> +void ccid_card_card_inserted(CCIDCardState *card);
> +void ccid_card_card_error(CCIDCardState *card, uint64_t error);
> +void ccid_card_qdev_register(CCIDCardInfo *card);
> +
> +/* support guest visible insertion/removal of ccid devices based on actual
> + * devices connected/removed. Called by card implementation (passthru, local) */
and here
> diff --git a/hw/usb-ccid.c b/hw/usb-ccid.c
> new file mode 100644
> index 0000000..bf4022a
> --- /dev/null
> +++ b/hw/usb-ccid.c
> +#define CCID_DEV_NAME "usb-ccid"
> +
> +/* The two options for variable sized buffers:
> + * make them constant size, for large enough constant,
> + * or handle the migration complexity - VMState doesn't handle this case.
> + * sizes are expected never to be exceeded, unless guest misbehaves. */
here again
[snip]
> +/* Using Gemplus Vendor and Product id
> + Effect on various drivers:
> + * usbccid.sys (winxp, others untested) is a class driver so it doesn't care.
> + * linux has a number of class drivers, but openct filters based on
> + vendor/product (/etc/openct.conf under fedora), hence Gemplus.
> + */
Something went totally boink with the comments there!
> +/* 6.2.6 RDR_to_PC_SlotStatus definitions */
> +enum {
> + CLOCK_STATUS_RUNNING = 0,
> + /* 0 - Clock Running, 1 - Clock stopped in State L, 2 - H,
> + 3 - unkonwn state. rest are RFU
> + */
> +};
> +
> +typedef struct __attribute__ ((__packed__)) CCID_Header {
> + uint8_t bMessageType;
> + uint32_t dwLength;
> + uint8_t bSlot;
> + uint8_t bSeq;
> +} CCID_Header;
Is this header decided upon by the CCID spec or the code? It seems
suboptimal to have a uint8 in front of a uint32 like that. Inefficient
structure alignment :(
> +
> +/* 6.1.4 PC_to_RDR_XfrBlock */
> +typedef struct __attribute__ ((__packed__)) CCID_XferBlock {
> + CCID_Header hdr;
> + uint8_t bBWI; /* Block Waiting Timeout */
> + uint16_t wLevelParameter; /* XXX currently unused */
> + uint8_t abData[0];
> +} CCID_XferBlock;
> +
> +typedef struct __attribute__ ((__packed__)) CCID_IccPowerOn {
> + CCID_Header hdr;
> + uint8_t bPowerSelect;
> + uint16_t abRFU;
> +} CCID_IccPowerOn;
Same problem with the above two structs....
> +typedef struct __attribute__ ((__packed__)) CCID_IccPowerOff {
> + CCID_Header hdr;
> + uint16_t abRFU;
> +} CCID_IccPowerOff;
> +
> +typedef struct __attribute__ ((__packed__)) CCID_SetParameters {
> + CCID_Header hdr;
> + uint8_t bProtocolNum;
> + uint16_t abRFU;
> + uint8_t abProtocolDataStructure[0];
> +} CCID_SetParameters;
and again.
> +/**
> + * powered - defaults to true, changed by PowerOn/PowerOff messages
> + */
> +struct USBCCIDState {
> + USBDevice dev;
> + CCIDBus *bus;
> + CCIDCardState *card;
> + CCIDCardInfo *cardinfo; /* caching the info pointer */
> + uint8_t debug;
> + BulkIn bulk_in_pending[BULK_IN_PENDING_NUM]; /* circular */
> + uint32_t bulk_in_pending_start;
> + uint32_t bulk_in_pending_end; /* first free */
> + uint32_t bulk_in_pending_num;
> + BulkIn *current_bulk_in;
> + uint8_t bulk_out_data[BULK_OUT_DATA_SIZE];
> + uint32_t bulk_out_pos;
> + uint8_t bmSlotICCState;
> + uint8_t powered;
> + uint8_t notify_slot_change;
> + uint64_t last_answer_error;
> + Answer pending_answers[PENDING_ANSWERS_NUM];
> + uint32_t pending_answers_start;
> + uint32_t pending_answers_end;
> + uint32_t pending_answers_num;
> + uint8_t bError;
> + uint8_t bmCommandStatus;
> + uint8_t bProtocolNum;
> + uint8_t abProtocolDataStructure[MAX_PROTOCOL_SIZE];
> + uint32_t ulProtocolDataStructureSize;
> + uint32_t state_vmstate;
> + uint8_t migration_state;
> + uint32_t migration_target_ip;
> + uint16_t migration_target_port;
> +};
Try to place the struct elements a little better so you don't end up
with a lot of space wasted due to natural alignment by the compiler.
> +static void ccid_bulk_in_get(USBCCIDState *s)
> +{
> + if (s->current_bulk_in != NULL || s->bulk_in_pending_num == 0) {
> + return;
> + }
> + assert(s->bulk_in_pending_num > 0);
> + s->bulk_in_pending_num--;
> + s->current_bulk_in = &s->bulk_in_pending[
> + (s->bulk_in_pending_start++) % BULK_IN_PENDING_NUM];
That line break is really not good :( Either break it after the '=' or
calculate the index outside the assignment statement.
> +static void ccid_write_data_block(
> + USBCCIDState *s, uint8_t slot, uint8_t seq,
> + const uint8_t *data, uint32_t len)
Please fix this - keep some arguments on the first line, and align the
following ones to match.
> +/* handle a single USB_TOKEN_OUT, return value returned to guest.
> + * 0 - all ok
> + * USB_RET_STALL - failed to handle packet */
another badly formatted comment
> +void ccid_card_card_error(CCIDCardState *card, uint64_t error)
> +{
> + USBCCIDState *s =
> + DO_UPCAST(USBCCIDState, dev.qdev, card->qdev.parent_bus->parent);
> +
> + s->bmCommandStatus = COMMAND_STATUS_FAILED;
> + s->last_answer_error = error;
> + DPRINTF(s, 1, "VSC_Error: %lX\n", s->last_answer_error);
> + /* TODO: these error's should be more verbose and propogated to the guest.
> + * */
> + /* we flush all pending answers on CardRemove message in ccid-card-passthru,
> + * so check that first to not trigger abort */
!!! there's more below.
Except for the mostly cosmetic stuff, it looks ok to me.
Cheers,
Jes
next prev parent reply other threads:[~2011-03-14 13:55 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-23 11:20 [Qemu-devel] [PATCH v20 0/7] usb-ccid Alon Levy
2011-02-23 11:20 ` [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus Alon Levy
2011-03-14 13:54 ` Jes Sorensen [this message]
2011-03-14 14:07 ` Daniel P. Berrange
2011-03-14 14:12 ` Anthony Liguori
2011-03-16 9:15 ` Alon Levy
2011-03-16 9:26 ` Jes Sorensen
2011-02-23 11:20 ` [Qemu-devel] [PATCH 2/7] introduce libcacard/vscard_common.h Alon Levy
2011-03-14 14:01 ` Jes Sorensen
2011-03-14 14:51 ` Alon Levy
2011-03-14 14:52 ` Alon Levy
2011-03-14 15:50 ` Jes Sorensen
2011-03-14 16:31 ` Alon Levy
2011-02-23 11:20 ` [Qemu-devel] [PATCH 3/7] ccid: add passthru card device Alon Levy
2011-03-14 14:04 ` Jes Sorensen
2011-03-14 14:53 ` Alon Levy
2011-03-14 15:51 ` Jes Sorensen
2011-02-23 11:20 ` [Qemu-devel] [PATCH 4/7] libcacard: initial commit Alon Levy
2011-03-14 15:20 ` Jes Sorensen
2011-03-14 16:40 ` Alon Levy
2011-03-15 12:42 ` Jes Sorensen
2011-03-15 13:14 ` Alon Levy
2011-03-15 13:40 ` Jes Sorensen
2011-03-15 14:09 ` Alon Levy
2011-03-15 13:45 ` Anthony Liguori
2011-03-15 14:23 ` Alon Levy
2011-03-16 8:23 ` Jes Sorensen
2011-03-16 8:40 ` Alon Levy
2011-03-16 8:42 ` Jes Sorensen
2011-03-15 13:44 ` Anthony Liguori
2011-03-15 14:25 ` Alon Levy
2011-03-15 14:51 ` Jes Sorensen
2011-03-15 14:56 ` Anthony Liguori
2011-03-15 14:59 ` Jes Sorensen
2011-03-15 15:14 ` Alon Levy
2011-03-16 8:26 ` Jes Sorensen
2011-03-15 14:55 ` Anthony Liguori
2011-03-17 13:36 ` Alon Levy
2011-02-23 11:20 ` [Qemu-devel] [PATCH 5/7] ccid: add ccid-card-emulated device Alon Levy
2011-03-14 15:41 ` Jes Sorensen
2011-03-14 16:44 ` Alon Levy
2011-03-14 17:11 ` Jes Sorensen
2011-03-17 10:54 ` Alon Levy
2011-03-17 10:59 ` Alon Levy
2011-03-17 14:25 ` Jes Sorensen
2011-02-23 11:20 ` [Qemu-devel] [PATCH 6/7] ccid: add docs Alon Levy
2011-03-14 15:41 ` Jes Sorensen
2011-02-23 11:20 ` [Qemu-devel] [PATCH 7/7] ccid: configure: improve --enable-smartcard flags Alon Levy
2011-03-14 15:44 ` Jes Sorensen
2011-03-06 10:50 ` [Qemu-devel] [PATCH v20 0/7] usb-ccid Alon Levy
-- strict thread matches above, loose matches on Subject: below --
2011-02-07 16:34 [Qemu-devel] [PATCH 0/7] usb-ccid (v19) Alon Levy
2011-02-07 16:34 ` [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus Alon Levy
2011-02-22 16:03 ` Anthony Liguori
2011-02-23 15:10 ` Alon Levy
2011-01-11 8:42 [Qemu-devel] [PATCH 0/7] usb-ccid (v15) Alon Levy
2011-01-11 8:42 ` [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus Alon Levy
2011-01-11 8:38 [Qemu-devel] [PATCH 0/7] usb-ccid (v14) Alon Levy
2011-01-11 8:38 ` [Qemu-devel] [PATCH 1/7] usb-ccid: add CCID bus Alon Levy
2011-01-25 14:10 ` Anthony Liguori
2011-01-25 16:10 ` Alon Levy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D7E1E33.1070801@redhat.com \
--to=jes.sorensen@redhat.com \
--cc=alevy@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.