On Tue, 2016-09-06 at 08:18 -0400, Frediano Ziglio wrote:
> >
> >
> > 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
> >
>
> This was why I used the priv[1] "trick", no allocation required, no
> free,
> allocation and "initialization" of priv is done while base object is
> allocated like in GObject.
> Considering that possibly that patches are going in a 0.14 version I
> would surely avoid the leak.
>
Sorry for the late reply. The only problem with the priv[1] trick is
that we can't use forward declaration for that. So we'll have to
temp0orarily expose some additional private data in some headers. But
you're right. it shouldn't leak.
> Frediano
>
> >
> > 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,
> > > FA
> > > LSE,
> > > nu
> > > m_common_caps,
> > > co
> > > mmon_caps,
> > > nu
> > > m_caps,
> > > ca
> > > ps);
> > > + 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
> >
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel