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@xxxxxxxxxx> > > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel