Hi,
On Wed, Aug 31, 2016 at 11:54:41AM -0500, Jonathon Jongsma wrote:
> Prepare to port to GObject by encapsulating all private data
If I'm not mistaken, the private struct now will leak while
SmartCardChannelClient is not a GObject with its finalize/dispose.
If that is the case, might point it out in the commit log and/or a FIXME
on g_new0
Besides that, looks ok to me.
Reviewed-by: Victor Toso <victortoso@xxxxxxxxxx>
> ---
> server/smartcard.c | 87 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 47 insertions(+), 40 deletions(-)
>
> diff --git a/server/smartcard.c b/server/smartcard.c
> index 74c2b18..8d22be4 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -49,8 +49,14 @@
> // Maximal length of APDU
> #define APDUBufSize 270
>
> +typedef struct SmartCardChannelClientPrivate SmartCardChannelClientPrivate;
> typedef struct SmartCardChannelClient {
> RedChannelClient base;
> +
> + SmartCardChannelClientPrivate *priv;
> +} SmartCardChannelClient;
> +
> +struct SmartCardChannelClientPrivate {
> RedCharDeviceSmartcard *smartcard;
>
> /* read_from_client/write_to_device buffer.
> @@ -58,7 +64,7 @@ typedef struct SmartCardChannelClient {
> RedCharDeviceWriteBuffer *write_buf;
> int msg_in_write_buf; /* was the client msg received into a RedCharDeviceWriteBuffer
> * or was it explicitly malloced */
> -} SmartCardChannelClient;
> +};
>
> G_DEFINE_TYPE(RedCharDeviceSmartcard, red_char_device_smartcard, RED_TYPE_CHAR_DEVICE)
>
> @@ -316,9 +322,9 @@ static void smartcard_char_device_attach_client(SpiceCharDeviceInstance *char_de
> RedCharDeviceSmartcard *dev = red_char_device_opaque_get(char_device->st);
> int client_added;
>
> - spice_assert(!scc->smartcard && !dev->priv->scc);
> + spice_assert(!scc->priv->smartcard && !dev->priv->scc);
> dev->priv->scc = scc;
> - scc->smartcard = dev;
> + scc->priv->smartcard = dev;
> client_added = red_char_device_client_add(RED_CHAR_DEVICE(dev),
> red_channel_client_get_client(&scc->base),
> FALSE, /* no flow control yet */
> @@ -330,7 +336,7 @@ static void smartcard_char_device_attach_client(SpiceCharDeviceInstance *char_de
> if (!client_added) {
> spice_warning("failed");
> dev->priv->scc = NULL;
> - scc->smartcard = NULL;
> + scc->priv->smartcard = NULL;
> red_channel_client_disconnect(&scc->base);
> }
> }
> @@ -361,14 +367,14 @@ static void smartcard_char_device_detach_client(SmartCardChannelClient *scc)
> {
> RedCharDeviceSmartcard *dev;
>
> - if (!scc->smartcard) {
> + if (!scc->priv->smartcard) {
> return;
> }
> - dev = scc->smartcard;
> + dev = scc->priv->smartcard;
> spice_assert(dev->priv->scc == scc);
> red_char_device_client_remove(RED_CHAR_DEVICE(dev),
> red_channel_client_get_client(&scc->base));
> - scc->smartcard = NULL;
> + scc->priv->smartcard = NULL;
> dev->priv->scc = NULL;
> }
>
> @@ -386,26 +392,26 @@ static uint8_t *smartcard_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> /* todo: only one reader is actually supported. When we fix the code to support
> * multiple readers, we will porbably associate different devices to
> * differenc channels */
> - if (!scc->smartcard) {
> - scc->msg_in_write_buf = FALSE;
> + if (!scc->priv->smartcard) {
> + scc->priv->msg_in_write_buf = FALSE;
> return spice_malloc(size);
> } else {
> RedCharDeviceSmartcard *dev;
>
> spice_assert(g_smartcard_readers.num == 1);
> - dev = scc->smartcard;
> - spice_assert(dev->priv->scc || scc->smartcard);
> - spice_assert(!scc->write_buf);
> - scc->write_buf = red_char_device_write_buffer_get(RED_CHAR_DEVICE(dev),
> - red_channel_client_get_client(rcc),
> - size);
> -
> - if (!scc->write_buf) {
> + dev = scc->priv->smartcard;
> + spice_assert(dev->priv->scc || scc->priv->smartcard);
> + spice_assert(!scc->priv->write_buf);
> + scc->priv->write_buf = red_char_device_write_buffer_get(RED_CHAR_DEVICE(dev),
> + red_channel_client_get_client(rcc),
> + size);
> +
> + if (!scc->priv->write_buf) {
> spice_error("failed to allocate write buffer");
> return NULL;
> }
> - scc->msg_in_write_buf = TRUE;
> - return scc->write_buf->buf;
> + scc->priv->msg_in_write_buf = TRUE;
> + return scc->priv->write_buf->buf;
> }
> }
>
> @@ -420,13 +426,13 @@ static void smartcard_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> * multiple readers, we will porbably associate different devices to
> * differenc channels */
>
> - if (!scc->msg_in_write_buf) {
> - spice_assert(!scc->write_buf);
> + if (!scc->priv->msg_in_write_buf) {
> + spice_assert(!scc->priv->write_buf);
> free(msg);
> } else {
> - if (scc->write_buf) { /* msg hasn't been pushed to the guest */
> - spice_assert(scc->write_buf->buf == msg);
> - red_char_device_write_buffer_release(RED_CHAR_DEVICE(scc->smartcard), &scc->write_buf);
> + if (scc->priv->write_buf) { /* msg hasn't been pushed to the guest */
> + spice_assert(scc->priv->write_buf->buf == msg);
> + red_char_device_write_buffer_release(RED_CHAR_DEVICE(scc->priv->smartcard), &scc->priv->write_buf);
> }
> }
> }
> @@ -467,7 +473,7 @@ static void smartcard_channel_send_migrate_data(RedChannelClient *rcc,
> SpiceMarshaller *m2;
>
> scc = SPICE_CONTAINEROF(rcc, SmartCardChannelClient, base);
> - dev = scc->smartcard;
> + dev = scc->priv->smartcard;
> red_channel_client_init_send_data(rcc, SPICE_MSG_MIGRATE_DATA, item);
> spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_SMARTCARD_MAGIC);
> spice_marshaller_add_uint32(m, SPICE_MIGRATE_DATA_SMARTCARD_VERSION);
> @@ -513,8 +519,8 @@ static void smartcard_channel_on_disconnect(RedChannelClient *rcc)
> {
> SmartCardChannelClient *scc = SPICE_CONTAINEROF(rcc, SmartCardChannelClient, base);
>
> - if (scc->smartcard) {
> - RedCharDeviceSmartcard *dev = scc->smartcard;
> + if (scc->priv->smartcard) {
> + RedCharDeviceSmartcard *dev = scc->priv->smartcard;
>
> smartcard_char_device_detach_client(scc);
> smartcard_char_device_notify_reader_remove(dev);
> @@ -578,13 +584,13 @@ static void smartcard_remove_reader(SmartCardChannelClient *scc, uint32_t reader
> VSC_GENERAL_ERROR);
> return;
> }
> - spice_assert(scc->smartcard == dev);
> + spice_assert(scc->priv->smartcard == dev);
> smartcard_char_device_notify_reader_remove(dev);
> }
>
> static void smartcard_add_reader(SmartCardChannelClient *scc, uint8_t *name)
> {
> - if (!scc->smartcard) { /* we already tried to attach a reader to the client
> + if (!scc->priv->smartcard) { /* we already tried to attach a reader to the client
> when it connected */
> SpiceCharDeviceInstance *char_device = smartcard_readers_get_unattached();
>
> @@ -595,7 +601,7 @@ static void smartcard_add_reader(SmartCardChannelClient *scc, uint8_t *name)
> }
> smartcard_char_device_attach_client(char_device, scc);
> }
> - smartcard_char_device_notify_reader_add(scc->smartcard);
> + smartcard_char_device_notify_reader_add(scc->priv->smartcard);
> // The device sends a VSC_Error message, we will let it through, no
> // need to send our own. We already set the correct reader_id, from
> // our RedCharDeviceSmartcard.
> @@ -614,7 +620,7 @@ static void smartcard_channel_write_to_reader(RedCharDeviceWriteBuffer *write_bu
> spice_assert(vheader->reader_id <= g_smartcard_readers.num);
> sin = g_smartcard_readers.sin[vheader->reader_id];
> dev = (RedCharDeviceSmartcard *)red_char_device_opaque_get(sin->st);
> - spice_assert(!dev->priv->scc || dev == dev->priv->scc->smartcard);
> + spice_assert(!dev->priv->scc || dev == dev->priv->scc->priv->smartcard);
> /* protocol requires messages to be in network endianess */
> vheader->type = htonl(vheader->type);
> vheader->length = htonl(vheader->length);
> @@ -623,8 +629,8 @@ static void smartcard_channel_write_to_reader(RedCharDeviceWriteBuffer *write_bu
> /* pushing the buffer to the write queue; It will be released
> * when it will be fully consumed by the device */
> red_char_device_write_buffer_add(sin->st, write_buf);
> - if (dev->priv->scc && write_buf == dev->priv->scc->write_buf) {
> - dev->priv->scc->write_buf = NULL;
> + if (dev->priv->scc && write_buf == dev->priv->scc->priv->write_buf) {
> + dev->priv->scc->priv->write_buf = NULL;
> }
> }
>
> @@ -676,7 +682,7 @@ static int smartcard_channel_client_handle_migrate_data(RedChannelClient *rcc,
> return TRUE;
> }
>
> - if (!scc->smartcard) {
> + if (!scc->priv->smartcard) {
> SpiceCharDeviceInstance *char_device = smartcard_readers_get_unattached();
>
> if (!char_device) {
> @@ -687,10 +693,10 @@ static int smartcard_channel_client_handle_migrate_data(RedChannelClient *rcc,
> }
> }
> spice_debug("reader added %d partial read_size %u", mig_data->reader_added, mig_data->read_size);
> - scc->smartcard->priv->reader_added = mig_data->reader_added;
> + scc->priv->smartcard->priv->reader_added = mig_data->reader_added;
>
> - smartcard_device_restore_partial_read(scc->smartcard, mig_data);
> - return red_char_device_restore(RED_CHAR_DEVICE(scc->smartcard), &mig_data->base);
> + smartcard_device_restore_partial_read(scc->priv->smartcard, mig_data);
> + return red_char_device_restore(RED_CHAR_DEVICE(scc->priv->smartcard), &mig_data->base);
> }
>
> static int smartcard_channel_handle_message(RedChannelClient *rcc,
> @@ -737,8 +743,8 @@ static int smartcard_channel_handle_message(RedChannelClient *rcc,
> vheader->type, vheader->length);
> return FALSE;
> }
> - spice_assert(scc->write_buf->buf == msg);
> - smartcard_channel_write_to_reader(scc->write_buf);
> + spice_assert(scc->priv->write_buf->buf == msg);
> + smartcard_channel_write_to_reader(scc->priv->write_buf);
>
> return TRUE;
> }
> @@ -760,6 +766,7 @@ static void smartcard_connect_client(RedChannel *channel, RedClient *client,
> FALSE,
> num_common_caps, common_caps,
> num_caps, caps);
> + scc->priv = g_new0(SmartCardChannelClientPrivate,1 );
>
> if (!scc) {
> return;
> @@ -818,7 +825,7 @@ red_char_device_smartcard_finalize(GObject *object)
>
> free(self->priv->buf);
> if (self->priv->scc) {
> - self->priv->scc->smartcard = NULL;
> + self->priv->scc->priv->smartcard = NULL;
> }
>
> G_OBJECT_CLASS(red_char_device_smartcard_parent_class)->finalize(object);
> --
> 2.7.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel