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

[Hans de Goede]

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 ?

Regards,

Hans