>
> On Thu, 2016-09-08 at 13:39 -0400, Frediano Ziglio wrote:
> > >
> > >
> > > Previously we were creating a variable named 'dev_state' and then
> > > apparently not using it. Well, we *were* actually using it, but in
> > > a
> > > convoluted sort of way. Creating a new RedCharDevice has a
> > > side-effect of setting itself as the 'st' attribute of
> > > SpiceCharDeviceInstance. So 'dev_state' and 'char_device->st' are
> > > in
> > > fact the same variable. But they were being used interchangeably,
> > > which
> > > was rather confusing. For example
> > >
> > > if (dev_state)
> > > // do something with char_device->st
> > >
> > > So this patch doesn't actually change anything, but it makes the
> > > code a
> > > bit easier to follow.
> >
> > You are perfectly right!
> >
> > >
> > > ---
> > > server/reds.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/server/reds.c b/server/reds.c
> > > index cc541a9..81b378c 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -3254,7 +3254,7 @@ static int
> > > spice_server_char_device_add_interface(SpiceServer *reds,
> > > }
> > >
> > > if (dev_state) {
> > > - spice_assert(char_device->st);
> > > + spice_assert(dev_state == char_device->st);
> >
> > Honestly I had spend quite some time checking this was right.
> > Maybe a comment like
> >
> > /* the code above create a state for the character device and they
> > should bound them */
> > spice_assert(dev_state == char_device->st);
> >
> > (I tried to came with something better...)
> >
>
>
> How about:
>
> /* When spicevmc_device_connect() is called to create a RedCharDevice,
> * it also assigns that as the internal state for char_device. This is
> * just a sanity check to ensure that assumption is correct */
> spice_assert(dev_state == char_device->st);
>
>
> > >
> > >
> > > g_object_weak_ref(G_OBJECT(dev_state),
> > > (GWeakNotify)reds_on_char_device_destroy
> > > ,
> > > @@ -3262,9 +3262,9 @@ static int
> > > spice_server_char_device_add_interface(SpiceServer *reds,
> > > /* setting the char_device state to "started" for backward
> > > compatibily with
> > > * qemu releases that don't call spice api for start/stop
> > > (not
> > > implemented yet) */
> > > if (reds->vm_running) {
> > > - red_char_device_start(char_device->st);
> > > + red_char_device_start(dev_state);
> > > }
> > > - reds_add_char_device(reds, char_device->st);
> > > + reds_add_char_device(reds, dev_state);
> > > } else {
> > > spice_warning("failed to create device state for %s",
> > > char_device->subtype);
> > > return -1;
> >
> > Otherwise,
> >
> > Acked-by: Frediano Ziglio <fzig...@redhat.com>
> >
Merged with the comment change
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
