[Spice-devel] [PATCH 01/10] RedsState: clean up spice_server_char_device_add_interface

[Jonathon Jongsma]

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 <fziglio at redhat.com>
> 
> Frediano