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

[Frediano Ziglio]

> 
> 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...)

>  
>          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