[Spice-devel] [PATCH 05/10] smartcard: Turn SmartcardState into a GObject
Jonathon Jongsma
jjongsma at redhat.com
Thu Mar 31 14:28:01 UTC 2016
On Thu, 2016-03-31 at 05:01 -0400, Frediano Ziglio wrote:
> > Here's a patch that I think could be squashed into this one:
> >
> > diff --git a/server/smartcard.c b/server/smartcard.c
> > index f592a9f..3d40f59 100644
> > --- a/server/smartcard.c
> > +++ b/server/smartcard.c
> > @@ -67,7 +67,6 @@ G_DEFINE_TYPE(RedCharDeviceSmartcard,
> > red_char_device_smartcard, RED_TYPE_CHAR_D
> > #define RED_CHAR_DEVICE_SMARTCARD_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE
> > ((o),
> > RED_TYPE_CHAR_DEVICE_SMARTCARD, RedCharDeviceSmartcardPrivate))
> >
> > struct RedCharDeviceSmartcardPrivate {
>
> Reading all your comments looks like the naming conventions are quite
> messy.
> Why RedCharDeviceSmartcardPrivate and SmartCardDeviceState?
> - we have "Smartcard" and "SmartCard";
> - "Red" prefix and no "Red" prefix;
> - one mention the char device the other not;
> - one contains "State" the other not.
> Not mentioning: why we need a Private part if all the implementation
> in contained in a single C file? Private to what?
>
> About "State". "State" is using in the public headers to distinguish
> between instance (ie SpiceCharDeviceInstance) and internal state
> (ie SpiceCharDeviceState). For internal classes there is no need to have
> "State" into the name.
This was basically the issue I raised here:
https://lists.freedesktop.org/archives/spice-devel/2016-March/027850.html
I think we should get rid of the SmartCardDeviceState type name completely since
it's just a typedef for RedCharDeviceSmartcard. We should use
RedCharDeviceSmartcard exclusively.
>
> About "Smartcard" and "SmartCard" I would prefer "Smartcard" however
> $ cgrep Smartcard | wc -l
> 33
> $ cgrep SmartCard | wc -l
> 53
> so looks like code prefers the "SmartCard" version.
>
> I would go for a "SmartCardDevice" possibly without the private part or use
> a "SmartCardDevicePrivate". Also macros are like RED_CHAR_DEVICE_SMARTCARD_xxx
> but should be SMARTCARD_DEVICE_xxx.
>
> About function prefixes on the same like should be smartcard_device_XXX
> and not smartcard_char_device_XXX.
>
> Frediano
>
>
> > - SpiceCharDeviceState *chardev_st;
> > uint32_t reader_id;
> > /* read_from_device buffer */
> > uint8_t *buf;
> > @@ -381,7 +380,7 @@ static void
> > smartcard_char_device_detach_client(SmartCardChannelClient *scc)
> > }
> > st = scc->smartcard_state;
> > spice_assert(st->priv->scc == scc);
> > - spice_char_device_client_remove(st->priv->chardev_st, scc
> > ->base.client);
> > + spice_char_device_client_remove(st, scc->base.client);
> > scc->smartcard_state = NULL;
> > st->priv->scc = NULL;
> > }
> >
> >
> > On Wed, 2016-03-30 at 14:29 -0500, Jonathon Jongsma wrote:
> > > See comments at
> > > https://lists.freedesktop.org/archives/spice-devel/2016-March/027850.html
> > >
> > > On Wed, 2016-03-30 at 18:21 +0100, Frediano Ziglio wrote:
> > > > From: Christophe Fergeau <cfergeau at redhat.com>
> > > >
> > > > This inherits from RedCharDevice. Once all char device states are
> > > > converted, we can turn the associated vfuncs into RedCharDeviceClass
> > > > vfuncs.
> > > >
> > > > Signed-off-by: Christophe Fergeau <cfergeau at redhat.com>
> > > > ---
> > > > server/smartcard.c | 125
> > > > ++++++++++++++++++++++++++++++--------------------
> > > > --
> > > > -
> > > > server/smartcard.h | 30 ++++++++++++-
> > > > 2 files changed, 100 insertions(+), 55 deletions(-)
> > > >
> > > > diff --git a/server/smartcard.c b/server/smartcard.c
> > > > index d34fa05..195f668 100644
> > > > --- a/server/smartcard.c
> > > > +++ b/server/smartcard.c
> > > > @@ -1,6 +1,6 @@
> > > > /* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > > /*
> > > > - Copyright (C) 2010 Red Hat, Inc.
> > > > + Copyright (C) 2010-2015 Red Hat, Inc.
> > > >
> > > > This library is free software; you can redistribute it and/or
> > > > modify it under the terms of the GNU Lesser General Public
> > > > @@ -49,7 +49,7 @@
> > > > // Maximal length of APDU
> > > > #define APDUBufSize 270
> > > >
> > > > -typedef struct SmartCardDeviceState SmartCardDeviceState;
> > > > +typedef RedCharDeviceSmartcard SmartCardDeviceState;
> > > >
> > > > typedef struct SmartCardChannelClient {
> > > > RedChannelClient base;
> > > > @@ -62,6 +62,10 @@ typedef struct SmartCardChannelClient {
> > > > * or was it explicitly malloced */
> > > > } SmartCardChannelClient;
> > > >
> > > > +G_DEFINE_TYPE(RedCharDeviceSmartcard, red_char_device_smartcard,
> > > > RED_TYPE_CHAR_DEVICE)
> > > > +
> > > > +#define RED_CHAR_DEVICE_SMARTCARD_PRIVATE(o)
> > > > (G_TYPE_INSTANCE_GET_PRIVATE
> > > > ((o), RED_TYPE_CHAR_DEVICE_SMARTCARD, RedCharDeviceSmartcardPrivate))
> > > > +
> > > > struct RedCharDeviceSmartcardPrivate {
> > > > SpiceCharDeviceState *chardev_st;
> > > > uint32_t reader_id;
> > > > @@ -75,10 +79,6 @@ struct RedCharDeviceSmartcardPrivate {
> > > > int reader_added; // has reader_add been sent to
> > > > the
> > > > device
> > > > };
> > > >
> > > > -struct SmartCardDeviceState {
> > > > - struct RedCharDeviceSmartcardPrivate priv[1];
> > > > -};
> > > > -
> > > > enum {
> > > > PIPE_ITEM_TYPE_ERROR = PIPE_ITEM_TYPE_CHANNEL_BASE,
> > > > PIPE_ITEM_TYPE_SMARTCARD_DATA,
> > > > @@ -122,7 +122,6 @@ static void
> > > > smartcard_channel_write_to_reader(SpiceCharDeviceWriteBuffer *write_
> > > > static MsgItem *smartcard_char_device_on_message_from_device(
> > > > SmartCardDeviceState *state, VSCMsgHeader *header);
> > > > static SmartCardDeviceState *smartcard_device_state_new(RedsState
> > > > *reds,
> > > > SpiceCharDeviceInstance *sin);
> > > > -static void smartcard_device_state_free(SmartCardDeviceState* st);
> > > > static void smartcard_init(RedsState *reds);
> > > >
> > > > static void smartcard_read_buf_prepare(SmartCardDeviceState *state,
> > > > VSCMsgHeader *vheader)
> > > > @@ -275,48 +274,33 @@ static SpiceCharDeviceInstance
> > > > *smartcard_readers_get_unattached(void)
> > > >
> > > > static SmartCardDeviceState *smartcard_device_state_new(RedsState
> > > > *reds,
> > > > SpiceCharDeviceInstance *sin)
> > > > {
> > > > - SmartCardDeviceState *st;
> > > > - SpiceCharDeviceCallbacks chardev_cbs = { NULL, };
> > > > -
> > > > - chardev_cbs.read_one_msg_from_device =
> > > > smartcard_read_msg_from_device;
> > > > - chardev_cbs.ref_msg_to_client = smartcard_ref_msg_to_client;
> > > > - chardev_cbs.unref_msg_to_client = smartcard_unref_msg_to_client;
> > > > - chardev_cbs.send_msg_to_client = smartcard_send_msg_to_client;
> > > > - chardev_cbs.send_tokens_to_client =
> > > > smartcard_send_tokens_to_client;
> > > > - chardev_cbs.remove_client = smartcard_remove_client;
> > > > -
> > > > - st = spice_new0(SmartCardDeviceState, 1);
> > > > - st->priv->chardev_st = spice_char_device_state_create(sin,
> > > > - reds,
> > > > - 0, /* tokens
> > > > interval
> > > > */
> > > > - ~0, /* self tokens
> > > > */
> > > > - &chardev_cbs,
> > > > - st);
> > > > - st->priv->reader_id = VSCARD_UNDEFINED_READER_ID;
> > > > - st->priv->reader_added = FALSE;
> > > > - st->priv->buf_size = APDUBufSize + sizeof(VSCMsgHeader);
> > > > - st->priv->buf = spice_malloc(st->priv->buf_size);
> > > > - st->priv->buf_pos = st->priv->buf;
> > > > - st->priv->buf_used = 0;
> > > > - st->priv->scc = NULL;
> > > > - return st;
> > > > -}
> > > > + RedCharDevice *char_dev;
> > > > + SpiceCharDeviceCallbacks char_dev_cbs = {
> > > > + .read_one_msg_from_device = smartcard_read_msg_from_device,
> > > > + .ref_msg_to_client = smartcard_ref_msg_to_client,
> > > > + .unref_msg_to_client = smartcard_unref_msg_to_client,
> > > > + .send_msg_to_client = smartcard_send_msg_to_client,
> > > > + .send_tokens_to_client = smartcard_send_tokens_to_client,
> > > > + .remove_client = smartcard_remove_client,
> > > > + };
> > > >
> > > > -static void smartcard_device_state_free(SmartCardDeviceState* st)
> > > > -{
> > > > - if (st->priv->scc) {
> > > > - st->priv->scc->smartcard_state = NULL;
> > > > - }
> > > > - free(st->priv->buf);
> > > > - spice_char_device_state_destroy(st->priv->chardev_st);
> > > > - free(st);
> > > > + char_dev = g_object_new(RED_TYPE_CHAR_DEVICE_SMARTCARD,
> > > > + "sin", sin,
> > > > + "spice-server", reds,
> > > > + "client-tokens-interval", 0ULL,
> > > > + "self-tokens", ~0ULL,
> > > > + NULL);
> > > > +
> > > > + red_char_device_set_callbacks(RED_CHAR_DEVICE(char_dev),
> > > > + &char_dev_cbs, char_dev);
> > > > + return RED_CHAR_DEVICE_SMARTCARD(char_dev);
> > > > }
> > > >
> > > > void smartcard_device_disconnect(SpiceCharDeviceInstance *char_device)
> > > > {
> > > > - SmartCardDeviceState *st =
> > > > spice_char_device_state_opaque_get(char_device
> > > > ->st);
> > > > + g_return_if_fail(RED_IS_CHAR_DEVICE_SMARTCARD(char_device));
> > > >
> > > > - smartcard_device_state_free(st);
> > > > + g_object_unref(char_device);
> > > > }
> > > >
> > > > SpiceCharDeviceState *smartcard_device_connect(RedsState *reds,
> > > > SpiceCharDeviceInstance *char_device)
> > > > @@ -325,10 +309,10 @@ SpiceCharDeviceState
> > > > *smartcard_device_connect(RedsState
> > > > *reds, SpiceCharDeviceI
> > > >
> > > > st = smartcard_device_state_new(reds, char_device);
> > > > if (smartcard_char_device_add_to_readers(reds, char_device) == -1)
> > > > {
> > > > - smartcard_device_state_free(st);
> > > > + g_object_unref(st);
> > > > return NULL;
> > > > }
> > > > - return st->priv->chardev_st;
> > > > + return RED_CHAR_DEVICE(st);
> > > > }
> > > >
> > > > static void
> > > > smartcard_char_device_notify_reader_add(SmartCardDeviceState
> > > > *st)
> > > > @@ -336,7 +320,7 @@ static void
> > > > smartcard_char_device_notify_reader_add(SmartCardDeviceState *st)
> > > > SpiceCharDeviceWriteBuffer *write_buf;
> > > > VSCMsgHeader *vheader;
> > > >
> > > > - write_buf = spice_char_device_write_buffer_get(st->priv
> > > > ->chardev_st,
> > > > NULL, sizeof(vheader));
> > > > + write_buf = spice_char_device_write_buffer_get(RED_CHAR_DEVICE(st),
> > > > NULL,
> > > > sizeof(vheader));
> > > > if (!write_buf) {
> > > > spice_error("failed to allocate write buffer");
> > > > return;
> > > > @@ -358,7 +342,7 @@ static void
> > > > smartcard_char_device_attach_client(SpiceCharDeviceInstance *char_de
> > > > spice_assert(!scc->smartcard_state && !st->priv->scc);
> > > > st->priv->scc = scc;
> > > > scc->smartcard_state = st;
> > > > - client_added = spice_char_device_client_add(st->priv->chardev_st,
> > > > + client_added = spice_char_device_client_add(RED_CHAR_DEVICE(st),
> > > > scc->base.client,
> > > > FALSE, /* no flow
> > > > control
> > > > yet
> > > > */
> > > > 0, /* send queue size
> > > > */
> > > > @@ -383,7 +367,7 @@ static void
> > > > smartcard_char_device_notify_reader_remove(SmartCardDeviceState *st)
> > > > spice_debug("reader add was never sent to the device");
> > > > return;
> > > > }
> > > > - write_buf = spice_char_device_write_buffer_get(st->priv
> > > > ->chardev_st,
> > > > NULL, sizeof(vheader));
> > > > + write_buf = spice_char_device_write_buffer_get(RED_CHAR_DEVICE(st),
> > > > NULL,
> > > > sizeof(vheader));
> > > > if (!write_buf) {
> > > > spice_error("failed to allocate write buffer");
> > > > return;
> > > > @@ -434,7 +418,7 @@ static uint8_t
> > > > *smartcard_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> > > > st = scc->smartcard_state;
> > > > spice_assert(st->priv->scc || scc->smartcard_state);
> > > > spice_assert(!scc->write_buf);
> > > > - scc->write_buf = spice_char_device_write_buffer_get(st->priv
> > > > ->chardev_st, rcc->client, size);
> > > > + scc->write_buf =
> > > > spice_char_device_write_buffer_get(RED_CHAR_DEVICE(st), rcc->client,
> > > > size);
> > > >
> > > > if (!scc->write_buf) {
> > > > spice_error("failed to allocate write buffer");
> > > > @@ -460,11 +444,9 @@ static void
> > > > smartcard_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> > > > spice_assert(!scc->write_buf);
> > > > free(msg);
> > > > } else {
> > > > - SpiceCharDeviceState *dev_st;
> > > > if (scc->write_buf) { /* msg hasn't been pushed to the guest */
> > > > spice_assert(scc->write_buf->buf == msg);
> > > > - dev_st = scc->smartcard_state ? scc->smartcard_state->priv
> > > > ->chardev_st : NULL;
> > > > - spice_char_device_write_buffer_release(dev_st,
> > > > scc->write_buf);
> > > > + spice_char_device_write_buffer_release(RED_CHAR_DEVICE(scc
> > > > ->smartcard_state), scc->write_buf);
> > > > scc->write_buf = NULL;
> > > > }
> > > > }
> > > > @@ -518,7 +500,7 @@ static void
> > > > smartcard_channel_send_migrate_data(RedChannelClient *rcc,
> > > > spice_marshaller_add_uint32(m, 0);
> > > > spice_debug("null char dev state");
> > > > } else {
> > > > - spice_char_device_state_migrate_data_marshall(state->priv
> > > > ->chardev_st, m);
> > > > +
> > > > spice_char_device_state_migrate_data_marshall(RED_CHAR_DEVICE(state),
> > > > m);
> > > > spice_marshaller_add_uint8(m, state->priv->reader_added);
> > > > spice_marshaller_add_uint32(m, state->priv->buf_used);
> > > > m2 = spice_marshaller_get_ptr_submarshaller(m, 0);
> > > > @@ -746,7 +728,7 @@ static int
> > > > smartcard_channel_client_handle_migrate_data(RedChannelClient *rcc,
> > > > scc->smartcard_state->priv->reader_added = mig_data->reader_added;
> > > >
> > > > smartcard_device_state_restore_partial_read(scc->smartcard_state,
> > > > mig_data);
> > > > - return spice_char_device_state_restore(scc->smartcard_state->priv
> > > > ->chardev_st, &mig_data->base);
> > > > + return spice_char_device_state_restore(RED_CHAR_DEVICE(scc
> > > > ->smartcard_state), &mig_data->base);
> > > > }
> > > >
> > > > static int smartcard_channel_handle_message(RedChannelClient *rcc,
> > > > @@ -871,3 +853,38 @@ static void smartcard_init(RedsState *reds)
> > > >
> > > > reds_register_channel(reds, &g_smartcard_channel->base);
> > > > }
> > > > +
> > > > +
> > > > +static void
> > > > +red_char_device_smartcard_finalize(GObject *object)
> > > > +{
> > > > + RedCharDeviceSmartcard *self = RED_CHAR_DEVICE_SMARTCARD(object);
> > > > +
> > > > + free(self->priv->buf);
> > > > + if (self->priv->scc) {
> > > > + self->priv->scc->smartcard_state = NULL;
> > > > + }
> > > > +
> > > > + G_OBJECT_CLASS(red_char_device_smartcard_parent_class)
> > > > ->finalize(object);
> > > > +}
> > > > +
> > > > +static void
> > > > +red_char_device_smartcard_class_init(RedCharDeviceSmartcardClass
> > > > *klass)
> > > > +{
> > > > + GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > > > +
> > > > + g_type_class_add_private(klass, sizeof
> > > > (RedCharDeviceSmartcardPrivate));
> > > > +
> > > > + object_class->finalize = red_char_device_smartcard_finalize;
> > > > +}
> > > > +
> > > > +static void
> > > > +red_char_device_smartcard_init(RedCharDeviceSmartcard *self)
> > > > +{
> > > > + self->priv = RED_CHAR_DEVICE_SMARTCARD_PRIVATE(self);
> > > > +
> > > > + self->priv->reader_id = VSCARD_UNDEFINED_READER_ID;
> > > > + self->priv->buf_size = APDUBufSize + sizeof(VSCMsgHeader);
> > > > + self->priv->buf = spice_malloc(self->priv->buf_size);
> > > > + self->priv->buf_pos = self->priv->buf;
> > > > +}
> > > > diff --git a/server/smartcard.h b/server/smartcard.h
> > > > index 32d2367..4b00433 100644
> > > > --- a/server/smartcard.h
> > > > +++ b/server/smartcard.h
> > > > @@ -1,6 +1,6 @@
> > > > /* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > > /*
> > > > - Copyright (C) 2010 Red Hat, Inc.
> > > > + Copyright (C) 2010-2015 Red Hat, Inc.
> > > >
> > > > This library is free software; you can redistribute it and/or
> > > > modify it under the terms of the GNU Lesser General Public
> > > > @@ -18,6 +18,34 @@
> > > > #ifndef __SMART_CARD_H__
> > > > #define __SMART_CARD_H__
> > > >
> > > > +#include <glib-object.h>
> > > > +
> > > > +#define RED_TYPE_CHAR_DEVICE_SMARTCARD
> > > > red_char_device_smartcard_get_type()
> > > > +
> > > > +#define RED_CHAR_DEVICE_SMARTCARD(obj)
> > > > (G_TYPE_CHECK_INSTANCE_CAST((obj),
> > > > RED_TYPE_CHAR_DEVICE_SMARTCARD, RedCharDeviceSmartcard))
> > > > +#define RED_CHAR_DEVICE_SMARTCARD_CLASS(klass)
> > > > (G_TYPE_CHECK_CLASS_CAST((klass), RED_TYPE_CHAR_DEVICE_SMARTCARD,
> > > > RedCharDeviceSmartcardClass))
> > > > +#define RED_IS_CHAR_DEVICE_SMARTCARD(obj)
> > > > (G_TYPE_CHECK_INSTANCE_TYPE((obj),
> > > > RED_TYPE_CHAR_DEVICE_SMARTCARD))
> > > > +#define RED_IS_CHAR_DEVICE_SMARTCARD_CLASS(klass)
> > > > (G_TYPE_CHECK_CLASS_TYPE((klass), RED_TYPE_CHAR_DEVICE_SMARTCARD))
> > > > +#define RED_CHAR_DEVICE_SMARTCARD_GET_CLASS(obj)
> > > > (G_TYPE_INSTANCE_GET_CLASS((obj), RED_TYPE_CHAR_DEVICE_SMARTCARD,
> > > > RedCharDeviceSmartcardClass))
> > > > +
> > > > +typedef struct RedCharDeviceSmartcard RedCharDeviceSmartcard;
> > > > +typedef struct RedCharDeviceSmartcardClass RedCharDeviceSmartcardClass;
> > > > +typedef struct RedCharDeviceSmartcardPrivate
> > > > RedCharDeviceSmartcardPrivate;
> > > > +
> > > > +struct RedCharDeviceSmartcard
> > > > +{
> > > > + RedCharDevice parent;
> > > > +
> > > > + RedCharDeviceSmartcardPrivate *priv;
> > > > +};
> > > > +
> > > > +struct RedCharDeviceSmartcardClass
> > > > +{
> > > > + RedCharDeviceClass parent_class;
> > > > +};
> > > > +
> > > > +GType red_char_device_smartcard_get_type(void) G_GNUC_CONST;
> > > > +
> > > > /*
> > > > * connect to smartcard interface, used by smartcard channel
> > > > */
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
More information about the Spice-devel
mailing list