On 03/16/11 10:15, Alon Levy wrote: > On Mon, Mar 14, 2011 at 02:54:59PM +0100, Jes Sorensen wrote: >>> +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 :( >> > > In the spec.
I was afraid of that, clearly a spec written by the people doing the wire protocol, without considering the software aspects. >>> +/** >>> + * 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. >> > > ok, this one's me. I'm really not sure except for stuff that goes on the wire > or get's allocated a bazillion times that this is worth the change in general, > but since I'm respinning anyway I'll do it. (unless you're saying it should be > a habit). If it is a one-off allocation, it's really not a big deal, but it is a good thing to keep in mind. In particular on non-x86 64 bit entities are normally 64 bit aligned. >>> +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. > > ok, after the =, but then I think the rest is >80, so it will neccessitate > another > break. If it was my code, I would calculate the index on the previous line in a tmp variable. It is a matter of personal preference of course. >>> +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. > > Is that a coding style thing I missed or personal preferance? my personal > preferance > here is the way it is, since it looks shorter/more readable, but I don't care > that > much. It is not written down :(, but it is common practice. I raise the issue exactly because it is much more readable the other way :) >> >>> +/* 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 >> > fixing them all. Excellent! >>> +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. > ? more badly formated comments? more todos? more flushing? Comments yeah. It doesn't affect the code, but for a new patch it really is better to get it straightened before it goes upstream. Cheers, Jes