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

[Alon Levy]

On Fri, Feb 04, 2011 at 09:42:21AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02/03/2011 08:02 PM, Alon Levy wrote:
> >uses VSC_Error with code==VSC_SUCCESS instead. This means
> >we need to track number of pending readers to distinguish
> >other reasons for VSC_Error, so adding _num_pending_reader_inserts.
> >---
> >  client/smartcard_channel.cpp |   27 +++++++++++++++++----------
> >  client/smartcard_channel.h   |    1 +
> >  2 files changed, 18 insertions(+), 10 deletions(-)
> >
> >diff --git a/client/smartcard_channel.cpp b/client/smartcard_channel.cpp
> >index e7f3870..0b6b2a1 100644
> >--- a/client/smartcard_channel.cpp
> >+++ b/client/smartcard_channel.cpp
> >@@ -45,6 +45,7 @@ public:
> >
> >  SmartCardChannel::SmartCardChannel(RedClient&  client, uint32_t id)
> >      : RedChannel(client, SPICE_CHANNEL_SMARTCARD, id, new SmartCardHandler(*this))
> >+    , _num_pending_reader_inserts(0)
> >  {
> >      SmartCardHandler* handler = static_cast<SmartCardHandler*>(get_message_handler());
> >
> >@@ -84,6 +85,8 @@ void SmartCardChannel::add_unallocated_reader(VReader* vreader, const char* name
> >      data->reader_id = VSCARD_UNDEFINED_READER_ID;
> >      data->name = spice_strdup(name);
> >      _unallocated_readers_by_vreader.insert(std::pair<VReader*, ReaderData*>(vreader, data));
> >+    _num_pending_reader_inserts++;
> >+    LOG_INFO("adding unallocated reader %p (pending %d)", data, _num_pending_reader_inserts);
> >  }
> >
> >  /** called upon the VSC_ReaderAddResponse
> >@@ -96,6 +99,7 @@ ReaderData* SmartCardChannel::add_reader(uint32_t reader_id)
> >      assert(unallocated>  0);
> >      data = _unallocated_readers_by_vreader.begin()->second;
> >      data->reader_id = reader_id;
> >+    LOG_INFO("adding %p->%d", data, reader_id);
> >      _readers_by_vreader.insert(
> >          std::pair<VReader*, ReaderData*>(data->vreader, data));
> >      assert(_readers_by_vreader.count(data->vreader) == 1);
> >@@ -393,21 +397,24 @@ void VSCMessageEvent::response(AbstractProcessLoop&  loop)
> >      ReaderData* data;
> >
> >      switch (_vheader->type) {
> >-        case (VSC_ReaderAddResponse):
> >-            data = _smartcard_channel->add_reader(_vheader->reader_id);
> >-            if (data->card_insert_pending) {
> >-                data->card_insert_pending = false;
> >-                _smartcard_channel->send_atr(data->vreader);
> >-            }
> >-            return;
> >-            break;
> >          case VSC_APDU:
> >              break;
> >          case VSC_Error:
> >              {
> >                  VSCMsgError *error = (VSCMsgError*)_vheader->data;
> >-                LOG_WARN("VSC Error: reader %d, code %d",
> >-                    _vheader->reader_id, error->code);
> >+                if (error->code == VSC_SUCCESS) {
> >+                    if (_smartcard_channel->_num_pending_reader_inserts>  0) {
> >+                        --_smartcard_channel->_num_pending_reader_inserts;
> >+                        data = _smartcard_channel->add_reader(_vheader->reader_id);
> >+                        if (data->card_insert_pending) {
> >+                            data->card_insert_pending = false;
> >+                            _smartcard_channel->send_atr(data->vreader);
> >+                        }
> >+                    }
> >+                } else {
> >+                    LOG_WARN("VSC Error: reader %d, code %d",
> >+                        _vheader->reader_id, error->code);
> >+                }
> >              }
> >              return;
> >          default:
> 
> 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

And same for any other insert/remove card/reader events. I can easily implement this as a queue on the client side. I'll send it as a later patch.

> 
> Regards,
> 
> Hans
>