[Spice-devel] [PATCH 4/8] client/smartcard: libcacard dropped ReaderAddResponse

[Hans de Goede]

Hi,

On 02/04/2011 12:27 PM, Alon Levy wrote:
> On Fri, Feb 04, 2011 at 10:11:59AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02/04/2011 09:39 AM, Alon Levy wrote:
>>
>> <snip>
>>
>>>> Hmm,
>>>>
>>>> This seems error prone. What if the other side is sending a success state for some
>>>> other reason? Either error->code == VSC_SUCCESS can only be send for the
>>>> adding a reader case and keeping track of num_pending_reader_inserts is
>>>> not necessary or this seems racy to me. Maybe add a VCS_READER_ADD_SUCCESS
>>>> error code ?
>>>
>>> Ok, I admit this whole counter was a way to quickly fix the problem that I was getting a second VSC_SUCCESS. The idea was to have a single outstanding request that needs an answer, thereby removing ambiguity. But the implementation doesn't do that, hence the hack of _num_pending_reader_inserts. To keep the protocol as is (btw I just *removed* such a message because I wanted to simplify the protocol - ReaderAddResponse. equivalent to the suggested error code) I could just serialize to:
>>>   client: request reader add
>>>   client: wait for response
>>>   server: send VSCError.code==VSC_SUCCESS
>>>   client: report card insert
>>>   cient:  wait for response
>>>
>>
>> That is still racy. What if a VSCError.code==VSC_SUCCESS is already in the pipe
>> from server to client for some other request send by the client earlier. Then
>> the client will report the card insert, when it receives the unrelated
>> VSCError.code==VSC_SUCCESS.
>>
> another way to state my previous response: the VSC_Error is only sent by the server when the client is expected to wait for it. it is only a response to one of:
>   card insert (atr)
>   card remove
>   reader insert
>   reader remove
>

I sort of understand. Let me put it this way looking at it from a pure
data communications pov. You can have 2 things:
1) A stop and wait flow control kind of situation, if this is the case you know
    a success report belongs to your last command, but then I see no need for
    num_pending_reader_inserts magic

2) A flow control situation where there is a window, iow there can be multiple
    outstanding requests, your num_pending_reader_inserts magic gives me the
    feeling (I don't know the details of the smartcard stuff). This is the
    case here. In which case you somehow need to know which request a success
    message acks -> id's

Given that you're getting 2 success messages, this feels like a multiple
outstanding requests case, and the num_pending_reader_inserts magic feels like
the wrong solution, as it assumes the first success message is the one you're
waiting for.

While nitpicking I would also rename VCSError to VCSStatus, as it also reports
successes.

Regards,

Hans