[Spice-devel] [PATCH 05/10] smartcard: Turn SmartcardState into a GObject
Frediano Ziglio
fziglio at redhat.com
Thu Mar 31 09:01:17 UTC 2016
> 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.
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